[Fiber] Sync mount and unmount #8634

acdlite · 2016-12-23T20:30:31Z

Ensures that syncUpdates resets the batching context so that sync updates are not downgraded to Task. This provides an escape hatch for code that relies on synchronous flushing of updates.

Top-level mount and unmount are now wrapped in syncUpdates. Should we do this for ART and native as well?

sebmarkbage · 2016-12-23T21:22:51Z

Can you explain what shouldDeferSyncUpdates does and the change you had to make to the internals to make this work?

How are life-cycles affected? Such as if you render in a life-cycle.

acdlite · 2016-12-23T21:29:09Z

Previously we used isPerformingWork to infer whether we're in a batch, so we know to downgrade Sync priority to Task. This PR introduces an explicit flag instead, shouldDeferSyncUpdates, which is equal to isPerformingWork in every case except when you call syncUpdates.

So the only observable difference is if you nest syncUpdates inside a batch (either a batchedUpdates or a lifecycle).

ReactDOMFiber wraps top-level mount and unmount with syncUpdates.

sebmarkbage · 2016-12-24T00:19:27Z

Sorry, I still don't understand. What happens in the observable case? Does the new test cover it? What happens if you used the old behavior?

acdlite · 2016-12-24T06:13:26Z

@sebmarkbage In the observable case, updates inside syncUpdates inside componentDidMount/Update triggers a recursive call to performWork. I'm glad you asked that question though because as I was writing a response I realized that it wasn't working properly as written... I rewrote the test to use componentDidMount instead of batchedUpdates and it broke :D

Latest commit enables recursive calls to performWork by stashing the previous state of the scheduler (just the necessary bits) at the beginning of the function and restoring it upon exiting.

A few other tests are now failing. I'll look at these some time in the next few days.

acdlite · 2016-12-24T06:15:09Z

To answer your other question, the old behavior was to always defer sync updates if we were already performing work. This was to prevent recursion, but this new feature requires it.

acdlite · 2016-12-28T23:55:58Z

This is now ready for review.

One problem I discovered is whether to allow synchronous updates during the begin phase (inside render and componentWillMount). I don't think it's possible to safely do this in Fiber because it's recursive. The only place where it's safe to recursively trigger a sync update is during the commit phase. Otherwise you can easily hit an infinite loop where work is endlessly performed and thrown out.

My solution was to downgrade such updates to Task and print a warning.

For legacy purposes. Only enabled in the DOM renderer. We can remove this in a future release when we enable incremental-by-default. This change is unobservable because syncUpdates actually schedules Task updates when it is called from inside another batch. The correct behavior is to recursively begin another batch of work. We will fix it in a subsequent commit.

acdlite · 2016-12-29T06:00:57Z

Cleaned up the commits a bit so it's (hopefully) easier to review.

bvaughn · 2016-12-29T15:19:28Z

src/renderers/dom/fiber/ReactDOMFiber.js

@@ -322,9 +324,15 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any,
    while (container.lastChild) {
      container.removeChild(container.lastChild);
    }
-    root = container._reactRootContainer = DOMRenderer.createContainer(container);
+    const newRoot = DOMRenderer.createContainer(container);


Given that syncUpdates immediately executes the callback you pass it, why is the new newRoot variable necessary?

To satisfy Flow, which complains otherwise. I'm guessing because it doesn't know that syncUpdates executes its callback synchronously.

bvaughn · 2016-12-29T15:26:33Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+            // completeWork, rather than unwind the stack, we can just restart
+            // from the root. Can't do that until then because without memoized
+            // props, the nodes higher up in the tree will rerender unnecessarily.
+            if (failedWork) {


You've already checked if (failedWork) above. This check seems unnecessary.

bvaughn · 2016-12-29T15:41:18Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-                performWork(TaskPriority);
-              }
+              // TODO: If we're not already performing work, schedule a
+              // deferred callback.


Curious why this is a TODO instead of actual code. Is it more difficult than it seems?

No it's not. Just outside the scope of this PR. It should be accompanied by tests.

bvaughn · 2016-12-29T15:49:56Z

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

+    ReactNoop.batchedUpdates(() => {
+      ReactNoop.syncUpdates(() => {
+        ReactNoop.render(<span />);
+        expect(ReactNoop.getChildren()).toEqual([span()]);


Nit: This test could produce a false positive if either batchedUpdates or syncUpdates happened to defer the callback. (Which I guess is super unlikely so maybe this doesn't matter.)

Not sure what you mean. All batchedUpdates does it make it so any sync updates are batched together using Task priority. syncUpdates does the opposite, where if you're already in a batch, it forces a synchronous flush.

The test only passes if line 347 flushes synchronously. So I'm not sure how you'd get a false positive.

If batchedUpdates didn't immediately execute the provided callback, the test would complete without any assertions (since it's not an async test) which could potentially give you a false positive if there was a failing assertion inside of the inner callback.

Oh I see. Yeah this test assumes that batchedUpdates works properly, because we have other tests that cover the behavior of batchedUpdates.

You could guard against this with:

it('can force synchronous updates with syncUpdates, even inside batchedUpdates', (done) => { ReactNoop.batchedUpdates(() => { ReactNoop.syncUpdates(() => { ReactNoop.render(<span />); expect(ReactNoop.getChildren()).toEqual([span()]); done(); }); }); });

But yeah I agree, probably not necessary. 😄

I don't have a strong opinion on it, so I'll add it to make you happy :D

bvaughn · 2016-12-29T16:36:22Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -345,7 +348,7 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
      firstEffect = finishedWork.firstEffect;
    }

-    prepareForCommit();
+    const commitInfo = prepareForCommit();


I'm curious what motivated this method signature change. At first I thought it might be to support nested calls to commitAllWork (between the call to prepareForCommit and resetAfterCommit). But I don't think that's a use-case we support. commitAllLifeCycles might generate more work, but commitAllHostEffects shouldn't- right?

I'm probably overlooking something. 😁

That's what I thought too but something in the DOM renderer does trigger a re-render in between prepareForCommit and resetAfterCommit, in one of the tests. I'll be honest and confess I don't know exactly why — something in the event system I'm guessing — but this seems like a less fragile solution than what we were doing previously (stashing values in global state), anyway.

If you revert the last commit you'll see the test that fails.

I reverted your most recent commit (79f01b2) and ran all tests, and you're right. Looking at one of the failing tests, it seems that unmounting from a componentWillUnmount hook eventually results in more sync priority work being scheduled.

Okay. You're right, this seems less fragile anyway. 👍

Oh right, I forgot that componentWillUnmount is called during the first pass. That explains it then.

This can only be caused by a bug in the renderer, but we should handle it gracefully anyway. Added a TODO to change capturedErrors and failedBoundaries so that they are sets of instances (stateNodes) rather than fibers, to avoid having to check the alternate. (This outside the scope of this PR.)

Currently we assume that performWork is never called recursively. Ideally that is the case. We shouldn't rely on recursion anywhere in Fiber. However, to support the use of syncUpdates as an opt-in to forced- synchronous updates, we need the ability to call performWork recursively. At the beginning of performWork, the state of the scheduler is saved; before exiting, that state is restored, so that the scheduler can continue where it left off. I've decided to use local variables to stash the previous state. We could also store them in a record and push it to a stack, but this approach has the advantage of being isolated to performWork.

Typically, a sync update is downgraded to Task priority if it's scheduled within another batch of work, e.g. within a lifecycle like componentDidMount. But syncUpdates should force sync updates to not be downgraded to Task. This will cause performWork to be called recursively.

Stack allows this, but in Fiber it may cause an infinite loop. Instead we'll print a warning and defer the update by giving it Task priority.

The DOM renderer assumes that resetAfterCommit is called after prepareForCommit without any nested commits in between. That may not be the case now that syncUpdates forces a nested update. To address, this changes the type of prepareForCommit to return a value which is later passed to resetAfterCommit.

bvaughn

This PR looks good to me.

gaearon · 2016-12-30T01:23:41Z

src/renderers/dom/fiber/ReactDOMFiber.js

    ReactBrowserEventEmitter.setEnabled(false);
-    selectionInformation = ReactInputSelection.getSelectionInformation();
+    return {


FWIW this is how @spicyj wrote it originally but @sebmarkbage wanted to avoid the allocation.

I see. I don't see a way to avoid it given that a sync update may occur inside componentWillUnmount. Unless we disallow sync updates there.

That actually might be a good idea... Interleaving commit phases could cause trouble.

Since nested interleaved commits are broken anyway, can we revert this?

Oops yes I meant to do that, forgot!

acdlite requested review from sophiebits and sebmarkbage December 23, 2016 20:30

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 23, 2016

acdlite force-pushed the fibersyncmount branch 4 times, most recently from 197b461 to 3080d32 Compare December 28, 2016 23:50

sebmarkbage mentioned this pull request Dec 29, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

bvaughn self-assigned this Dec 29, 2016

acdlite force-pushed the fibersyncmount branch from 3080d32 to 2f85a21 Compare December 29, 2016 06:00

acdlite requested a review from bvaughn December 29, 2016 06:01

bvaughn reviewed Dec 29, 2016

View reviewed changes

bvaughn removed their assignment Dec 29, 2016

acdlite added 3 commits December 29, 2016 10:23

acdlite force-pushed the fibersyncmount branch from 2f85a21 to 3a25252 Compare December 29, 2016 18:24

acdlite added 2 commits December 29, 2016 10:39

Disallow forced sync updates during begin phase

ce8c978

Stack allows this, but in Fiber it may cause an infinite loop. Instead we'll print a warning and defer the update by giving it Task priority.

acdlite force-pushed the fibersyncmount branch from 3a25252 to 79f01b2 Compare December 29, 2016 18:39

bvaughn approved these changes Dec 29, 2016

View reviewed changes

acdlite merged commit 9b01518 into facebook:master Dec 29, 2016

acdlite added GH Review: accepted and removed GH Review: review-needed labels Dec 29, 2016

gaearon reviewed Dec 30, 2016

View reviewed changes

acdlite mentioned this pull request Dec 30, 2016

[Fiber] Introduce API to opt-out of batching #8661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Sync mount and unmount #8634

[Fiber] Sync mount and unmount #8634

acdlite commented Dec 23, 2016 •

edited

Loading

sebmarkbage commented Dec 23, 2016

acdlite commented Dec 23, 2016

sebmarkbage commented Dec 24, 2016

acdlite commented Dec 24, 2016

acdlite commented Dec 24, 2016

acdlite commented Dec 28, 2016

acdlite commented Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn Dec 29, 2016 •

edited

Loading

acdlite Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016 •

edited

Loading

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn Dec 29, 2016

acdlite Dec 29, 2016

bvaughn left a comment

gaearon Dec 30, 2016

acdlite Dec 30, 2016 •

edited

Loading

acdlite Dec 30, 2016

sebmarkbage Jan 11, 2017

acdlite Jan 11, 2017

[Fiber] Sync mount and unmount #8634

[Fiber] Sync mount and unmount #8634

Conversation

acdlite commented Dec 23, 2016 • edited Loading

sebmarkbage commented Dec 23, 2016

acdlite commented Dec 23, 2016

sebmarkbage commented Dec 24, 2016

acdlite commented Dec 24, 2016

acdlite commented Dec 24, 2016

acdlite commented Dec 28, 2016

acdlite commented Dec 29, 2016

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

bvaughn Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Dec 29, 2016 • 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

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Dec 30, 2016 • 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

acdlite commented Dec 23, 2016 •

edited

Loading

bvaughn Dec 29, 2016 •

edited

Loading

acdlite Dec 29, 2016 •

edited

Loading

acdlite Dec 30, 2016 •

edited

Loading