Fix idle priority task during animation by AsturaPhoenix · Pull Request #116422 · flutter/flutter

AsturaPhoenix · 2022-12-02T14:30:45Z

This PR is based on #82781 but takes a slightly different approach where instead of polling the task queue on the Timer queue, it waits until after the post-frame callback so that it doesn't compete with animation tasks scheduled via scheduleFrameCallback.

This also attempts to address the comments on the test by mimicking the calls at work and cleaning up earlier tests rather than using a real animator, at the expense of possible fidelity. An integration test may be warranted.

BASK.Trip.Planner.-.Google.Chrome.2022-12-02.06-07-46.mp4

This video shows this patch at work in https://github.com/AsturaPhoenix/trip_planner_aquamarine / http://34.83.198.158/trip_planner_aquamarine. Here, the marker z-ordering is changed depending on which tab is active, and the change is an expensive operation that is scheduled with Priority.animation - 1.

Issues: #82016

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

jonahwilliams · 2022-12-02T18:19:38Z

It looks like you may have run dart format on the framework code. We don't use machine formatting, there is more information on how we do format in the style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

Please revert these formatting changes so it is easier to review your patch

Adds a test exercising `scheduleTask` with `Priority.idle` while a `Ticker` is going off. This test demonstrates the issue by failing and is fixed by the next commit. Also necessitates cleaning up one of the previous tests to execute microtasks since warmup frames have an async, and simply running the exposed handler leaves dirty state.

Schedules tasks with priority < `Priority.animation` to possibly execute as a post-frame callback. Tasks with priority >= `Priority.animation` are executed at that time, and if there are no scheduled transient tasks awaiting the next frame then tasks with lower priority are executed as well. This has also been tested visually in an app.

The scheduler expects raw time to be monotonic, so avoid using time in the new test since it's not required.

AsturaPhoenix · 2022-12-02T21:14:40Z

@jonahwilliams Thanks; done.

Also fixed the test failure under randomized ordering by avoiding incrementing time during the new test. There seems to be a complex interaction in resetEpoch that I thought it best not to touch for now.

This helps ensure that the queues are drained even if the test fails.

goderbauer · 2023-01-04T00:18:45Z

      }
      return _taskQueue.isNotEmpty;
+    } else {
+      addPostFrameCallback((Duration timeStamp) {


Does this actually fix the problem of tasks not running, though? Adding a postframecallback does not force a frame. So, if currently no frame is running or scheduled, this may never run and tasks will never execute. I believe, there's no guarantee that a frame is running or scheduled at this point?

It looks like a frame would be guaranteed under the default schedulingStrategy, which may only return false if there are transient frame callbacks, which implies a call to scheduleFrame.

I think if we'd want to relax this assumption to allow schedulingStrategys that could return false when there is not a frame scheduled, we'd need some notion of when to try again. If we were to define such a condition to be at the next vsync, the fix could be as simple as scheduling a frame here (and we'd want to add a note to the SchedulingStrategy documentation). Otherwise, if we don't define such a notion, I think we'd be forced to spin-poll as in #82781.

If we do decide this assumption is fine, we can add a note to SchedulingStrategy and assert(_hasScheduledFrame || !framesEnabled) here.

For now I've just added a comment to SchedulingStrategy specifying the current behavior, that the strategy is re-evaluated after the next (or current) frame, or when a new task is scheduled. I haven't added any new assertions. I think this may be the least impactful way to go.

It is odd to rely on the inner workings of the default scheduling strategy here, because that one could be fully replaced.

I don't have a great immediate idea of what to do here instead, though. Maybe @Hixie has some advice?

A different way of looking at it with the documentation change is that rather than relying on the inner workings of the default scheduling strategy, we're defining that when a scheduling strategy returns false, tasks won't be re-evaluated again until "after the next (or current) frame, or until a new task is scheduled."

This shifts the implementation detail dependence so that it's contained within the expectations of the scheduling strategy itself, such that this patch would fix the expectation in the context of the default scheduling strategy. Were a scheduling strategy to be defined that returns false even if there are no frames scheduled, it would then not intrinsically have an expectation that the task/frame would be guaranteed to be scheduled, which I think is okay.

Right, using addPostFrameCallback doesn't work in the case where, say, the scheduling strategy is "do tasks except on Tuesdays". On a Tuesday, the scheduler strategy would return false, but on Wednesday we wouldn't know to retrigger the task queue.

I wonder if anyone has ever used the scheduling strategy logic. If not, we could just remove it and hard-code our current strategy and then the assumption would be safe and addPostFrameCallback would work.

I don't think, with the current API, that adding addPostFrameCallback is a good solution because it prevents us from feeling the pain of the broken API (without making the API less broken).

Add a comment to `SchedulingStrategy` specifying that skipped tasks are re-evaluated after the next or current frame, or after a new task is scheduled.

AsturaPhoenix · 2023-01-09T21:00:47Z

There's another thing that warrants discussion. _ensureEventLoopCallback schedules the event servicing loop using Timer.run. However, in widget tests this can leave us with a somewhat spurious "A Timer is still pending even after the widget tree was disposed." error. (We do need to ensure that the task we scheduled doesn't try to operate on the widget tree while !mounted, though.)

  void _ensureEventLoopCallback() {
    assert(!locked);
    assert(_taskQueue.isNotEmpty);
    if (_hasRequestedAnEventLoopCallback) {
      return;
    }
    _hasRequestedAnEventLoopCallback = true;
    Timer.run(_runTasks);
  }

WidgetTester.pumpAndSettle gets us past this, but if an animation was indeed scheduled, then we need at least another await tester.pump(Duration.zero); afterwards to service the post-frame timer introduced by this pull request, and this complicates the ability to actually write a widget test to catch the case where a task might try to operate on a disposed widget.

A full solution could be to make tasks cancellable and dispose the timer if all pending tasks are cancelled, but that doesn't seem worth the complexity.

We can get around this by scheduling the event loop as a microtask, though this does effectively increase its priority somewhat. I don't think the effect should be otherwise noticeable.

However, I'm inclined to just live with this. Thoughts?

Example of what such a test needs to look like:

    await tester.pumpWidget(MyWidget());
    await tester.pumpAndSettle();
    await tester.pump(Duration.zero);
    expect(find.byType(MyChild), findsOneWidget);

Example of a test that to catch a callback that runs after the widget has been disposed:

    await tester.pumpWidget(MyWidget());
    await tester.pumpAndSettle();
    // dispose
    await tester.pumpWidget(const SizedBox());
    // make sure the widget doesn't attempt to setState after disposal
    await tester.pump(Duration.zero);

Piinks · 2023-04-27T18:08:08Z

(Triage) what is the status of this PR? It looks like there has not been an update in a while here, @AsturaPhoenix is this waiting on another review?

AsturaPhoenix · 2023-04-27T18:50:50Z

@Piinks Yes; @goderbauer raised a concern that I think stems from the limitations of the API.

Please be advised that although I have this issue patched on my fork, my project is no longer hitting this issue.

I was using this to deal with it being expensive to change z-ordering of a large number of markers on Google Maps, but this is no longer an issue now that we've migrated to flutter_map. Google Maps seems to be doing a lot of unfortunate serialization, particularly on Android, where it's harder to avoid as it happens partly in GMS.

AsturaPhoenix · 2023-06-20T23:05:31Z

I'm inclined to close this PR since we no longer require this behavior.

flutter-dashboard Bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 2, 2022

AsturaPhoenix changed the title ~~Scheduler~~ Fix idle priority task during animation Dec 2, 2022

AsturaPhoenix mentioned this pull request Dec 2, 2022

Tasks scheduled through 'SchedulerBinding.instance.scheduleTask' were not executed #82016

Closed

AsturaPhoenix force-pushed the scheduler branch from 8eddebc to 53ccc63 Compare December 2, 2022 14:50

AsturaPhoenix added 3 commits December 2, 2022 11:51

Fix new test interaction under randomized ordering

4d9dc5a

The scheduler expects raw time to be monotonic, so avoid using time in the new test since it's not required.

AsturaPhoenix force-pushed the scheduler branch from 53ccc63 to 4d9dc5a Compare December 2, 2022 21:08

Move queue drain in scheduler test to tearDown

5eff422

This helps ensure that the queues are drained even if the test fails.

goderbauer self-requested a review December 6, 2022 23:18

goderbauer requested a review from dnfield January 4, 2023 00:13

goderbauer reviewed Jan 4, 2023

View reviewed changes

Specify when 'SchedulingStrategy' is reevaluated

30d621c

Add a comment to `SchedulingStrategy` specifying that skipped tasks are re-evaluated after the next or current frame, or after a new task is scheduled.

AsturaPhoenix closed this Jun 20, 2023

lauweijie mentioned this pull request Jul 2, 2024

[SchedulerBinding] SchedulerBinding.instance.scheduleTask is jammed when Priority is idle #73766

Closed

Navigation Menu

Repository navigation

Fix idle priority task during animation - #116422

Fix idle priority task during animation#116422
AsturaPhoenix wants to merge 5 commits into
flutter:masterfrom
AsturaPhoenix:scheduler

AsturaPhoenix commented Dec 2, 2022 •

edited

Loading

Uh oh!

jonahwilliams commented Dec 2, 2022

Uh oh!

AsturaPhoenix commented Dec 2, 2022

Uh oh!

goderbauer Jan 4, 2023

Uh oh!

AsturaPhoenix Jan 4, 2023 •

edited

Loading

Uh oh!

AsturaPhoenix Jan 4, 2023 •

edited

Loading

Uh oh!

goderbauer Feb 7, 2023

Uh oh!

AsturaPhoenix Feb 7, 2023

Uh oh!

Hixie Jun 20, 2023

Uh oh!

AsturaPhoenix commented Jan 9, 2023 •

edited

Loading

Uh oh!

Piinks commented Apr 27, 2023

Uh oh!

AsturaPhoenix commented Apr 27, 2023

Uh oh!

AsturaPhoenix commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

AsturaPhoenix commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams commented Dec 2, 2022

Uh oh!

AsturaPhoenix commented Dec 2, 2022

Uh oh!

goderbauer Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

AsturaPhoenix Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsturaPhoenix Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

AsturaPhoenix Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Hixie Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

AsturaPhoenix commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Apr 27, 2023

Uh oh!

AsturaPhoenix commented Apr 27, 2023

Uh oh!

AsturaPhoenix commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AsturaPhoenix commented Dec 2, 2022 •

edited

Loading

AsturaPhoenix Jan 4, 2023 •

edited

Loading

AsturaPhoenix Jan 4, 2023 •

edited

Loading

AsturaPhoenix commented Jan 9, 2023 •

edited

Loading