Request `DartPerformanceMode.latency` during transitions #110600

iskakaushik · 2022-08-30T17:58:54Z

This change makes it so that a call is made to request the latency performance mode, added in flutter/engine#35614 during transitions. It is to be noted that any requests made to this are not guaranteed to be honored.

This is an initial version of the change where there are no guardrails against indefinitely requesting the latency performance mode, these will be added as needed in follow-up PRs.

packages/flutter/lib/src/scheduler/performance_mode.dart

packages/flutter/test/scheduler/performance_mode_test.dart

iskakaushik · 2022-08-30T18:23:37Z

Is there a way to ignore a test file for web platform?

jonahwilliams · 2022-08-30T18:35:41Z

skip: kIsWeb ?

jonahwilliams · 2022-08-30T18:55:02Z

The problem is a compile error, ui.DartPerformanceMode is missing members on the web implementation. You'll need to add those in before this can land - and also file a bug for the API verification check missing it.

iskakaushik · 2022-08-30T18:58:25Z

The problem is a compile error, ui.DartPerformanceMode is missing members on the web implementation. You'll need to add those in before this can land - and also file a bug for the API verification check missing it.

Done: flutter/engine#35806

jonahwilliams · 2022-08-30T19:21:31Z

Once that rolls, you won't need to skip this on the web since its just a no-op there

packages/flutter/lib/src/scheduler/binding.dart

goderbauer · 2022-08-30T18:40:19Z

packages/flutter/lib/src/scheduler/binding.dart

+  /// Even if the result is `true`, the engine may choose to ignore the request or
+  /// the performance mode may not be guaranteed to be the one requested.
+  ///
+  /// If conflicting requests are made, only the first request will be honored.


Why the first? Shouldn't it be the "highest" one?

This was the situation I was trying to guard against:

C1 requests latency mode, and it is activated.

C2 requests throughput mode, we will deny this request, and not record the request anywhere.

In the current design, the primary situation I'm concerned with is the transitions requesting latency mode. if there are multiple conflicting requests, I am not adding them to any queue, only one of two things are supported now:

If no performance mode request is active, a component can request a specific performance mode.

If a performance mode P is active, another component can extend the lease on only P until dispose is called.

I think supporting a queue of requests is not necessary for the use-case that I'm solving. Hope this answers #110600 (comment) as well.

I see. Can you document that behavior on the method? And we can also simplify the implementation then. We only need to keep track of the active mode and how many handles we have given out for it. No need to imply with the implementation that we can keep track of different modes.

Also, as a side effect this means if somebody where to request balanced all other requests are blocked, which seems a little odd. When nobody has requested a mode we also run in balanced and we are happy to switch to any other mode.

packages/flutter/lib/src/scheduler/binding.dart

goderbauer · 2022-09-01T17:18:24Z

packages/flutter/lib/src/widgets/routes.dart

@@ -249,6 +257,8 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
    _animation = createAnimation()
      ..addStatusListener(_handleStatusChanged);
    assert(_animation != null, '$runtimeType.createAnimation() returned null.');
+    _performanceModeRequestHandle =
+      SchedulerBinding.instance.createPerformanceModeRequest(this, ui.DartPerformanceMode.latency);


nit: indent by two spaces.

It's already indented with 2 spaces, maybe i'm missing something?

packages/flutter/lib/src/widgets/routes.dart

packages/flutter/lib/src/scheduler/binding.dart

goderbauer · 2022-09-01T17:30:03Z

packages/flutter/lib/src/scheduler/binding.dart

+  /// Even if the result is `true`, the engine may choose to ignore the request or
+  /// the performance mode may not be guaranteed to be the one requested.
+  ///
+  /// If conflicting requests are made, only the first request will be honored.


I see. Can you document that behavior on the method? And we can also simplify the implementation then. We only need to keep track of the active mode and how many handles we have given out for it. No need to imply with the implementation that we can keep track of different modes.

Also, as a side effect this means if somebody where to request balanced all other requests are blocked, which seems a little odd. When nobody has requested a mode we also run in balanced and we are happy to switch to any other mode.

packages/flutter/lib/src/scheduler/binding.dart

jonahwilliams

LGTM with some nits

packages/flutter/lib/src/scheduler/binding.dart

jonahwilliams · 2022-09-02T17:33:15Z

packages/flutter/lib/src/scheduler/binding.dart

+/// See also:
+///
+/// * [PerformanceModeRequestHandle] for more information on the lifecycle of the handle.
+typedef PerformanceModeCleaupCallback = void Function();


Since this type isn't used anywhere, it can be a VoidCallback instead

It is still used in PerformanceModeRequestHandle, I think this typedef makes it easier to see when this callback will be invoked.

Sorry, I meant that the type is not used in a public APi, so this can be private or use an existing type - right?

Ah right, will change it to private.

packages/flutter/lib/src/scheduler/binding.dart

packages/flutter/lib/src/widgets/routes.dart

packages/flutter/test/scheduler/performance_mode_test.dart

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

goderbauer

LGTM

goderbauer · 2022-09-07T16:43:47Z

packages/flutter/lib/src/scheduler/binding.dart

+/// This callback is invoked when a request for [DartPerformanceMode] is disposed.
+///
+/// See also:
+///
+/// * [PerformanceModeRequestHandle] for more information on the lifecycle of the handle.
+typedef _PerformanceModeCleaupCallback = VoidCallback;


You can just delete this and use VoidCallback directly wherever _PerformanceModeCleaupCallback is currently used.

I think it's useful for the reader, makes it easy to understand that this type is called when the performance mode is resolved. Let me know if you feel strongly about it, and I can change it up in a follow-up PR.

…tter/flutter#110600)

…lutter/flutter#110600)

flutter-dashboard bot added f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. labels Aug 30, 2022

iskakaushik requested review from jonahwilliams and goderbauer August 30, 2022 17:59

jonahwilliams reviewed Aug 30, 2022

View reviewed changes

goderbauer reviewed Aug 30, 2022

View reviewed changes

iskakaushik force-pushed the transitions-gc branch from 5a0db7c to 2973fcb Compare August 31, 2022 16:05

iskakaushik added 4 commits August 31, 2022 16:33

Request a [DartPerformanceMode.latency] during transitions.

fc08e0e

address CR comments

9b988da

add validation for performance mode disposal

5042ced

createPerformanceModeRequest returns a PerformanceModeRequestHandle

327c3ec

iskakaushik force-pushed the transitions-gc branch from 2973fcb to 327c3ec Compare August 31, 2022 20:34

iskakaushik added 2 commits August 31, 2022 17:39

fix docs

b65ce6e

fix test failures

47a31ce

goderbauer reviewed Sep 1, 2022

View reviewed changes

iskakaushik added 3 commits September 1, 2022 14:22

partially address code review comments

a2c45fb

fix more failures

59e57c5

fix remaining threads

0a097d2

jonahwilliams approved these changes Sep 2, 2022

View reviewed changes

goderbauer reviewed Sep 2, 2022

View reviewed changes

iskakaushik and others added 5 commits September 6, 2022 11:09

Update packages/flutter/lib/src/scheduler/binding.dart

6d2f821

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

Update packages/flutter/lib/src/scheduler/binding.dart

317e22a

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

Update packages/flutter/lib/src/scheduler/binding.dart

0ad9650

Co-authored-by: Jonah Williams <jonahwilliams@google.com>

address changes except for writing a test

af6b55e

change cleanup callback to private

633d54c

iskakaushik force-pushed the transitions-gc branch from eb76fea to 633d54c Compare September 6, 2022 15:41

add a test to check performance mode during transitions

583eb78

goderbauer approved these changes Sep 7, 2022

View reviewed changes

iskakaushik merged commit d5f372b into flutter:master Sep 7, 2022

iskakaushik deleted the transitions-gc branch September 7, 2022 16:54

engine-flutter-autoroll mentioned this pull request Sep 7, 2022

Roll Flutter from 723b82e4f0f3 to ad1e76a20939 (31 revisions) flutter/plugins#6373

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022

d5f372b Request DartPerformanceMode.latency during transitions (flu…

db3f6e0

…tter/flutter#110600)

engine-flutter-autoroll mentioned this pull request Sep 8, 2022

Roll Flutter from 58ec4839159e to 66c306b45228 (31 revisions) flutter/packages#2581

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 8, 2022

d5f372bcc Request DartPerformanceMode.latency during transitions (f…

5620522

…lutter/flutter#110600)

engine-flutter-autoroll mentioned this pull request Sep 8, 2022

Roll Flutter from 58ec4839159e to fc444aedab12 (32 revisions) flutter/packages#2582

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 8, 2022

d5f372bcc Request DartPerformanceMode.latency during transitions (f…

c47ee7c

…lutter/flutter#110600)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request `DartPerformanceMode.latency` during transitions #110600

Request `DartPerformanceMode.latency` during transitions #110600

iskakaushik commented Aug 30, 2022

iskakaushik commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

iskakaushik commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

goderbauer Aug 30, 2022

iskakaushik Aug 31, 2022

goderbauer Sep 1, 2022

goderbauer Sep 1, 2022

iskakaushik Sep 1, 2022

goderbauer Sep 1, 2022

jonahwilliams left a comment

jonahwilliams Sep 2, 2022

iskakaushik Sep 6, 2022

jonahwilliams Sep 6, 2022

iskakaushik Sep 6, 2022

goderbauer left a comment

goderbauer Sep 7, 2022

iskakaushik Sep 7, 2022

Request DartPerformanceMode.latency during transitions #110600

Request DartPerformanceMode.latency during transitions #110600

Conversation

iskakaushik commented Aug 30, 2022

iskakaushik commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

iskakaushik commented Aug 30, 2022

jonahwilliams commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Request `DartPerformanceMode.latency` during transitions #110600

Request `DartPerformanceMode.latency` during transitions #110600