[Fiber] Don't schedule class fibers without relevant lifecycles for commit #9105

gaearon · 2017-03-03T19:49:39Z

While working on #9071, I noticed that we're scheduling Update effect for all classes, regardless of whether they have something to do in the commit phase:

(The JSONArrow class doesn't have lifecycle methods.)

To fix this, I am checking whether there's work to do before marking a fiber with the Update tag. This mirrors the logic in the commit phase.

To verify, I ran the same code with the new bundle, and now only class fibers with corresponding lifecycles are scheduled in the effect list:

This is a perf optimization and is unobservable from tests.

sebmarkbage · 2017-03-03T19:54:19Z

Seems like this removed a lot of overhead. That's a bit worrying since that indicates that we have a lot of overhead in the commit phase or in the perf measurement. We should probably make a comparison of running with and without perf instrumentation.

gaearon · 2017-03-03T19:58:36Z

Yea I'll definitely be doing that, it's on my list.
I'm not to worried because those are microseconds.

gaearon · 2017-03-03T20:01:18Z

With measurements on, the difference between before and after is 2ms vs 1ms. Is that enough to worry about?

trueadm · 2017-03-03T20:09:30Z

@gaearon Totally. Especially given you're probably perf testing on a top-end MacBook Pro. When I did perf testing with Inferno, I'd always run perf benchmarks again on an old crappy laptop I had, it always gave me a better indication on actual performance the end-users might get. Maybe we should get into the habit of doing something similar?

gaearon · 2017-03-03T20:23:12Z

Yeah, I just mean that I'd expect this to be ~1ms change on my example, so I don't think it's perf measurements exaggerating the issue. But I'll check if they are.

xtuc · 2017-03-03T20:29:10Z

Wouldn't it be interessting to test on a mobile device? What about RN?

gaearon · 2017-03-03T20:31:31Z

This is only relevant for Fiber codebase, it won't affect React apps using existing React version. It's also not that faster, just shaving off some minor unnecessary work in the algorithm we haven't optimized yet.

sebmarkbage · 2017-03-03T20:56:03Z

The commit phase will often be running at the very end of a frame deadline. 1ms is 6% of the entire frame. Easily enough to push it over the edge to miss the frame. So yea, I actually think that 1ms matters a lot in this particular case.

However, if that commit phase is invoking a bunch of componentDidMount/componentDidUpdate those are probably going to be pretty slow running and likely cause a frame drop anyway. Therefore the best solution is to just not use those like this PR helps with.

If these updates are on native nodes for example, then it is much more likely to be an issue since we will have a lots of those updates in normal frame updates.

trueadm · 2017-03-03T21:13:16Z

@gaearon @sebmarkbage Just an idea:

What if componentDidMount/componentDidUpdate were async by default? If we do that, we're less likely to push frames over the edge?

We could add componentDidMountSync/componentDidUpdateSync to support legacy apps for those that need them.

sebmarkbage · 2017-03-03T21:28:10Z

@trueadm Yea, I think we should make a revamp of the life-cycles. It's filed under #7678. I called the new one componentWasMounted for the async version since it is new behavior but I'm starting to think that maybe need a whole new naming scheme where the "default" shorter name is the async ones.

It's harder to break in place because there's no incremental path to fixing things.

acdlite · 2017-03-03T21:31:18Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+    if (current) {
+      // During an update, we might either need to detach a ref
+      // or to call the componentDidUpdate() hook in the commit phase.
+      const currentRef = current.ref;


We have a separate effect for refs (Ref)

The Ref effect gets marked here: https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberBeginWork.js#L273-L274

Or wait, should ref change trigger a componentDidUpdate call? I assumed no but maybe it should.

It should not.

trueadm · 2017-03-03T21:32:14Z

@sebmarkbage Thanks for the pointer on #7678. I'll comment there :)

acdlite

Refs shouldn't cause an Update effect.

acdlite · 2017-03-03T21:32:44Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+    if (current) {
+      // During an update, we might either need to detach a ref
+      // or to call the componentDidUpdate() hook in the commit phase.
+      const currentRef = current.ref;


The Ref effect gets marked here: https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberBeginWork.js#L273-L274

sebmarkbage

Yea, @acdlite is right but since @gaearon added this code, I wonder what lead to that. Is there a path where the Ref tag is insufficient?

gaearon · 2017-03-03T22:03:53Z

since @gaearon added this code, I wonder what lead to that. Is there a path where the Ref tag is insufficient?

I was just looking at what commitWork does for class components. We go into commitWork for anything with Update or PlacementAndUpdate effects.

acdlite · 2017-03-03T22:04:34Z

I believe the reason this didn't break any tests is because by the time this code is fired, shouldComponentUpdate did not return false, which means we will definitely re-render. So adding an Update effect is wrong but it doesn't cause a behavior change.

acdlite · 2017-03-03T22:06:01Z

@gaearon Oh yup, that's a bug. It should check for a Ref effect, too. Which means that switch statement will get a bit more complicated :D

sebmarkbage · 2017-03-03T22:10:45Z

Maybe we should just move that out to commitRef and check for Ref separately?

acdlite · 2017-03-03T23:10:07Z

src/renderers/shared/fiber/ReactFiberScheduler.js

      // Use Task priority for lifecycle updates
-      if (nextEffect.effectTag & (Update | Callback)) {
+      if (effectTag & (Update | Callback)) {


Outside the scope of this PR, but we should probably have a separate check for callbacks, for similar reasons.

acdlite · 2017-03-03T23:11:15Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+    if (!current && typeof instance.componentDidMount !== 'function') {
+      return;
+    }
+    if (current && typeof instance.componentDidUpdate !== 'function') {


Style q: why are these separate checks?

How would you combine them? It feels a bit messy to me to have || and && with ! in a single condition.

Yeah I can see how that looks a bit messy.

if ((!current && typeof instance.componentDidMount !== 'function') || (current && typeof instance.componentDidUpdate !== 'function')) { return; }

Alternatively, you could do

if (current) { if (typeof instance.componentDidUpdate !== 'function') { return; } } else { if (typeof instance.componentDidMount !== 'function') { return; } }

I tried the second one earlier but I thought it's a bit less confusing when it exits if we collapse them.

Perfwise is it the same? Does the engine have to check current twice?

Also we should use current === null

acdlite · 2017-03-03T23:22:12Z

src/renderers/shared/fiber/ReactFiberScheduler.js

        config.resetTextContent(nextEffect.stateNode);
      }

+      if (effectTag & Ref) {


I wonder if we should move this check into the switch statement. Already bad that ContentReset isn't in there. It requires adding a bunch of combinations (e.g. PlacementAndRef, PlacementAndUpdateAndRef, UpdateAndRef, etc) but it's better for perf.

gaearon · 2017-03-03T23:47:47Z

Anything more that should be done here? Should I do the explosion of flags in this PR?

acdlite · 2017-03-04T00:19:11Z

Personally I'm ok with it being in a separate PR

sebmarkbage · 2017-03-04T00:32:38Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+  function markUpdateIfNecessary(workInProgress) {
+    const current = workInProgress.alternate;
+    const instance = workInProgress.stateNode;
+    if (current !== null) {


Everywhere you call this, you already have the instance in scope (probably a register) and know whether it is an update or a mount. I'd recommend just inlining this stuff. It will be less abstractions to read when you follow the code and easier to optimize because all these field reads and checks disappear.

Don't schedule class fibers without relevant lifecycles for commit

63d92a9

gaearon requested review from sophiebits, bvaughn, sebmarkbage, trueadm and acdlite March 3, 2017 19:50

facebook-github-bot added the CLA Signed label Mar 3, 2017

acdlite reviewed Mar 3, 2017

View reviewed changes

acdlite requested changes Mar 3, 2017

View reviewed changes

sebmarkbage requested changes Mar 3, 2017

View reviewed changes

gaearon added 2 commits March 3, 2017 22:54

Separate Update and Ref effects

1b6fbf5

Simplify the exit condition

38bc823

acdlite reviewed Mar 3, 2017

View reviewed changes

Add missing annotation

55e4bdb

acdlite reviewed Mar 3, 2017

View reviewed changes

Write conditions differently to avoid an extra check

67a0768

acdlite approved these changes Mar 4, 2017

View reviewed changes

sebmarkbage reviewed Mar 4, 2017

View reviewed changes

sebmarkbage approved these changes Mar 4, 2017

View reviewed changes

gaearon added 2 commits March 4, 2017 04:01

Inline markUpdateIfNecessary()

34b7cc1

Inline markUpdateIfAlreadyInProgress()

1293e75

gaearon force-pushed the avoid-extra-updates branch from 2ac948b to 1293e75 Compare March 4, 2017 04:07

gaearon merged commit d87c4d5 into facebook:master Mar 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Don't schedule class fibers without relevant lifecycles for commit #9105

[Fiber] Don't schedule class fibers without relevant lifecycles for commit #9105

gaearon commented Mar 3, 2017 •

edited

Loading

sebmarkbage commented Mar 3, 2017

gaearon commented Mar 3, 2017

gaearon commented Mar 3, 2017

trueadm commented Mar 3, 2017

gaearon commented Mar 3, 2017

xtuc commented Mar 3, 2017

gaearon commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017 •

edited

Loading

trueadm commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017 •

edited by gaearon

Loading

acdlite Mar 3, 2017

acdlite Mar 3, 2017

acdlite Mar 3, 2017

sebmarkbage Mar 3, 2017

trueadm commented Mar 3, 2017

acdlite left a comment

acdlite Mar 3, 2017

sebmarkbage left a comment

gaearon commented Mar 3, 2017

acdlite commented Mar 3, 2017

acdlite commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017

acdlite Mar 3, 2017

acdlite Mar 3, 2017

gaearon Mar 3, 2017

acdlite Mar 3, 2017

gaearon Mar 3, 2017

acdlite Mar 3, 2017

gaearon Mar 3, 2017

acdlite Mar 3, 2017

gaearon commented Mar 3, 2017

acdlite commented Mar 4, 2017

sebmarkbage Mar 4, 2017 •

edited

Loading

[Fiber] Don't schedule class fibers without relevant lifecycles for commit #9105

[Fiber] Don't schedule class fibers without relevant lifecycles for commit #9105

Conversation

gaearon commented Mar 3, 2017 • edited Loading

sebmarkbage commented Mar 3, 2017

gaearon commented Mar 3, 2017

gaearon commented Mar 3, 2017

trueadm commented Mar 3, 2017

gaearon commented Mar 3, 2017

xtuc commented Mar 3, 2017

gaearon commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017 • edited Loading

trueadm commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017 • edited by gaearon 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

trueadm commented Mar 3, 2017

acdlite left a 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

gaearon commented Mar 3, 2017

acdlite commented Mar 3, 2017

acdlite commented Mar 3, 2017

sebmarkbage commented Mar 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Mar 3, 2017

acdlite commented Mar 4, 2017

sebmarkbage Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon commented Mar 3, 2017 •

edited

Loading

sebmarkbage commented Mar 3, 2017 •

edited

Loading

sebmarkbage commented Mar 3, 2017 •

edited by gaearon

Loading

sebmarkbage Mar 4, 2017 •

edited

Loading