Remove Animation priority #9968

acdlite · 2017-06-14T21:43:38Z

Follow up to this comment: #9952 (comment)

If we're already performing work, we shouldn't schedule an additional callback, e.g. if setState is called inside componentWillReceiveProps.

Update

The scope of this PR increased from its original description. Now it removes Animation priority as a feature. There's no advantage to scheduling updates with animation priority versus scheduling sync work inside requestAnimationCallback. So we can remove all the animation-specific code. Now there's only one type of callback.

acdlite · 2017-06-14T21:47:01Z

The first commit isn't super-related. It adds a new ReactNoop API for testing, which I used to test this bug fix. The commit message has details.

gaearon · 2017-06-14T22:45:01Z

Breaks the build for noop-renderer:

'jest-matchers' is imported by src/renderers/noop/ReactNoopEntry.js, but could not be resolved – treating it as an external dependency

Maybe need to add it here.

Allows us to make assertions on the values that are yielded when performing work. In our existing tests, we do this manually by pushing into an array. ReactNoop.flushThrough extends this concept. It accepts an array of expected values and flushes until those values are yielded.

sebmarkbage · 2017-06-15T22:45:08Z

yarn.lock

+  dependencies:
+    chalk "^1.1.3"
+    pretty-format "^20.0.3"
+
 jest-matchers@^19.0.0:


We have to download two of these questionable deps? Great.

I can change the version constraint ¯\_(ツ)_/¯ It's just the noop renderer

sebmarkbage · 2017-06-15T22:49:38Z

src/renderers/art/ReactARTFiberEntry.js

-  scheduleAnimationCallback: ReactDOMFrameScheduling.rAF,
-
-  scheduleDeferredCallback: ReactDOMFrameScheduling.rIC,
+  scheduleCallback: ReactDOMFrameScheduling.rIC,


This is too vague. scheduleDeferredCallback?

We could call it requestIdleCallback but I suspect we don't actually want to be forced to pass the concept of a deadline to this but rather ask it "shouldIContinue()`? To allow for environments that are not timer based but input based.

scheduleAsyncCallback? Seems redundant, but so does "deferred" :D Will we ever schedule more than one type of callback?

Remember that this is a public API for renderers to use. It should first and foremost describe what kind of function they're supposed to provide here.

Async isn't quite enough since that could be a micro-task tick (like the Promise queue).

Let's just call it scheduleDeferredCallback for now but idk how descriptive that actually is.

sebmarkbage · 2017-06-15T22:53:13Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+        switch (nextPriorityLevel) {
+          case SynchronousPriority:
+          case TaskPriority:
+            // We have remaining syncrhonous or task work. Keep performing it,


sebmarkbage · 2017-06-15T23:25:27Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-
+    let keepPerformingWork = true;
+    let hasRemainingAsyncWork = false;
+    while (keepPerformingWork && fatalError === null) {


What's the intention here with all the possible states (9) these can be in? I have trouble following the extra flags. The flags are indicative of a code smell. Can you refactor this in terms of control flow?

Basically this is a result of me trying to both 1) avoid while (true), and 2) not repeat the same checks that are done in workLoop. Should I just use while (true) and break / continue?

I could get rid of hasRemainingAsyncWork by scheduling the callback inside this loop instead of once we exit it. That's how I did it originally but I found this easier to follow. I'll change it back.

sebmarkbage · 2017-06-15T23:33:29Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-            case TaskPriority:
-              // TODO: If we're not already performing work, schedule a
-              // deferred callback.
-              return;


Did this return get lost? The TODO above seems wrong to me.

This is the topic I brought up at lunch. I believe you're the one who suggested this to me, and why I added this comment :)

Seems correct to me that if we setState at Task priority, but we're not already performing work, we should schedule a callback and perform it later. The other option is to treat it as synchronous work, I suppose. We need to decide whether "task" priority should be treated as semantically different from sync priority for non-nested updates.

This would schedule an unnecessary deferred callback if you call setState in componentDidMount inside a sync render.

Would be nice to have a test for that like Dan's ReactIncrementalPerf-test.js.snap that spawned these other changes.

Added a snapshot test

sebmarkbage · 2017-06-15T23:35:23Z

src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js

@@ -309,37 +267,6 @@ describe('ReactIncrementalScheduling', () => {
    expect(ReactNoop.getChildren()).toEqual([span(5)]);
  });

-  it('does not perform animation work after time runs out', () => {


Isn't this still a valuable test but for HighPriority instead of AnimationPriority?

Maybe? Why didn't we already have a HighPriority test then? My impression was that this test existed because of the different semantics for Animation priority versus async/deferred priorities.

sebmarkbage · 2017-06-15T23:39:47Z

src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js

    class Foo extends React.Component {
      state = {};
-      componentDidUpdate() {


Why delete this from the assertion? It has nothing to do with the ReactNoop.yield stuff so you've lost a part of the test, no?

I asserted on the children instead.

expect(ReactNoop.getChildren()).toEqual([span('')]);

There's no advantage to scheduling updates with animation priority versus scheduling sync work inside requestAnimationCallback. So we can remove all the animation-specific code. Now there's only one type of callback.

acdlite · 2017-06-16T00:16:14Z

@sebmarkbage Updated

sebmarkbage

Add unit tests for the things we found might break. :)

sebmarkbage · 2017-06-16T00:40:17Z

src/renderers/shared/fiber/ReactFiberScheduler.js

    }
  }

-  function performWork(
-    priorityLevel: PriorityLevel,


We realized that this is necessary but missing a unit test.

sebmarkbage · 2017-06-16T00:41:37Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-            case TaskPriority:
-              // TODO: If we're not already performing work, schedule a
-              // deferred callback.
-              return;


This would schedule an unnecessary deferred callback if you call setState in componentDidMount inside a sync render.

Would be nice to have a test for that like Dan's ReactIncrementalPerf-test.js.snap that spawned these other changes.

acdlite · 2017-06-16T18:11:11Z

@sebmarkbage I think I addressed everything

sebmarkbage · 2017-06-16T21:03:26Z

src/renderers/shared/fiber/ReactFiberScheduler.js

    const previousIsBatchingUpdates = isBatchingUpdates;
+    // This is only true if we're nested inside either batchedUpdates or a
+    // lifecycle method.
+    isUnbatchingUpdates = isBatchingUpdates || isPerformingWork;


Why is this set if isPerformingWork and then the consequence is ignored if isPerformingWork?

You're right, this check is redundant. I added this check first, then realized it made more sense to check isPerformingWork in scheduleUpdate, but forgot to remove the check here. I'll remove it.

Behavior now matches Stack. It's unfortunate that this prevents us from unifying SynchronousPriority and TaskPriority.

sebmarkbage · 2017-06-16T21:16:02Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+                  // batchedUpdates or a lifecycle. We should only flush
+                  // synchronous work, not task work.
+                  performWork(SynchronousPriority, null);
+                } else {


Why is this branch needed? Why aren't we just scheduling with TaskPriority if it is supposed to be Task priority?

Discussed offline, but documenting here for anyone watching.

Both branches are performing synchronous work. What's different is the first parameter to performWork, which specifies the minimum level of work that should be performed. In the normal case, we should perform both task and synchronous work. In the isUnbatchingUpdates case, we should only perform synchronous work.

sebmarkbage

Ok. I hope this is correct. Let's keep iterating.

acdlite requested review from sophiebits, sebmarkbage and gaearon June 14, 2017 21:43

facebook-github-bot added the CLA Signed label Jun 14, 2017

acdlite force-pushed the nestedupdatescallback branch from d0d37df to ba3e089 Compare June 14, 2017 21:44

acdlite force-pushed the nestedupdatescallback branch from ba3e089 to 98bb9ba Compare June 15, 2017 22:36

acdlite force-pushed the nestedupdatescallback branch from 98bb9ba to 0375526 Compare June 15, 2017 22:40

acdlite changed the title ~~Do not schedule a callback if already inside a callback~~ Remove Animation priority Jun 15, 2017

acdlite force-pushed the nestedupdatescallback branch from 0375526 to 27aefc5 Compare June 15, 2017 23:07

sebmarkbage reviewed Jun 15, 2017

View reviewed changes

Remove Animation priority

acaf7b2

There's no advantage to scheduling updates with animation priority versus scheduling sync work inside requestAnimationCallback. So we can remove all the animation-specific code. Now there's only one type of callback.

acdlite force-pushed the nestedupdatescallback branch from 27aefc5 to acaf7b2 Compare June 16, 2017 00:15

sebmarkbage requested changes Jun 16, 2017

View reviewed changes

acdlite force-pushed the nestedupdatescallback branch from d3b1b85 to dabfaf5 Compare June 16, 2017 18:12

sebmarkbage reviewed Jun 16, 2017

View reviewed changes

Task work inside batched updates is always sync, even for initial mounts

045a9cb

Behavior now matches Stack. It's unfortunate that this prevents us from unifying SynchronousPriority and TaskPriority.

acdlite force-pushed the nestedupdatescallback branch from dabfaf5 to 045a9cb Compare June 16, 2017 21:10

sebmarkbage reviewed Jun 16, 2017

View reviewed changes

Only check minPriorityLevel after a commit

db0c13f

sebmarkbage approved these changes Jun 17, 2017

View reviewed changes

acdlite closed this Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Animation priority #9968

Remove Animation priority #9968

acdlite commented Jun 14, 2017 •

edited

Loading

acdlite commented Jun 14, 2017

gaearon commented Jun 14, 2017

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017 •

edited

Loading

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017

sebmarkbage Jun 15, 2017

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017 •

edited

Loading

sebmarkbage Jun 15, 2017

acdlite Jun 16, 2017

sebmarkbage Jun 16, 2017

acdlite Jun 16, 2017

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017

sebmarkbage Jun 15, 2017

acdlite Jun 15, 2017

acdlite commented Jun 16, 2017

sebmarkbage left a comment

sebmarkbage Jun 16, 2017

sebmarkbage Jun 16, 2017

acdlite commented Jun 16, 2017

sebmarkbage Jun 16, 2017

acdlite Jun 16, 2017

acdlite Jun 16, 2017

sebmarkbage Jun 16, 2017

acdlite Jun 17, 2017

sebmarkbage left a comment

Remove Animation priority #9968

Remove Animation priority #9968

Conversation

acdlite commented Jun 14, 2017 • edited Loading

Update

acdlite commented Jun 14, 2017

gaearon commented Jun 14, 2017

Choose a reason for hiding this comment

acdlite Jun 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

acdlite Jun 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Jun 16, 2017

sebmarkbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Jun 16, 2017

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

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Jun 14, 2017 •

edited

Loading

acdlite Jun 15, 2017 •

edited

Loading

acdlite Jun 15, 2017 •

edited

Loading