[Fiber] Compute the Host Diff During Reconciliation #8607

sebmarkbage · 2016-12-20T03:32:13Z

This uses prepareUpdate to compute the diff to apply. If there is no work to do, we don't schedule any updates at all for the commit phase.

Unfortunately wrapper components have custom logic that gets applied in the commit phase so for now we need to always schedule an update for those.

The payload has an array of only the prop key and values that need to update.

This makes the commit phase in the Sierpinski Triangle demo go from 2.5-3ms to <1.5ms. The overall time probably increases but at least now we won't risk dropping frames as we're updating low-pri content.

A neat side-effect of this is that we no longer depend on any "contextual information" like host context or root instance in the commit phase.

TODO 1: ~~This doesn't correctly apply the diff when the is property switches from custom component mode to built-in component mode. I'll just need to deal with that when deleting values.~~ Errr... This is not true. We can't switch between modes. I just added some unused code at some point because I thought we could.

TODO 2: A more fundamental problem is that we always need to update the pointer to the "current" fiber atm because of how the lazy event listener thing works. I could probably change it to store the props object instead of going through the fiber.

~~TODO 3: Flow infinite loops. (Updating to 0.37.1 doesn't help. #8608)~~ Fixed using workaround.

sebmarkbage · 2016-12-20T03:37:24Z

src/renderers/shared/shared/event/EventPluginHub.js

@@ -127,6 +127,10 @@ var EventPluginHub = {
    // live here; needs to be moved to a better place soon
    if (typeof inst.tag === 'number') {
      const props = inst.memoizedProps;
+      if (!props) {
+        // Work in progress.


This is part of TODO 2 because if we have no event listeners, then we don't update the fiber pointer. If we don't update the Fiber pointer, then the new one becomes work in progress. If we try to read from work in progress, this might be null. If it completes, it might have event listeners on it that are not yet committed.

Therefore this isn't a great solution.

Seems like maybe this PR comment should be an inline comment so it doesn't get lost or forgotten?

gaearon · 2017-01-11T21:21:02Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+          } else {
+            (updatePayload = updatePayload || []).push(propKey, '' + nextHtml);
+          }
+        } else {


This should probably be (from #8652):

if (nextHtml != null) { if (lastHtml !== nextHtml) { (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); } } else { // TODO: ...

gaearon · 2017-01-11T23:35:54Z

(I used GH interface for resolving the conflict.)

sebmarkbage · 2017-01-11T23:36:34Z

I already rebased locally.

gaearon · 2017-01-11T23:36:57Z

:-( I should've asked.

gaearon · 2017-01-12T00:01:15Z

@sebmarkbage

Did you use a time machine writing these commits? 😄

To the next person utterly confused by GitHub's display of this PR, ba11c3b depends on 6ae87cf but for some reason was committed a day earlier, and thus appears before it in the list.

sebmarkbage · 2017-01-12T00:04:43Z

Like politics, rebasing is all about rewriting history. In chronological order or not.

gaearon · 2017-01-12T00:17:11Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+        for (styleName in lastStyle) {
+          if (lastStyle.hasOwnProperty(styleName)) {
+            if (!styleUpdates) {
+              styleUpdates = {};


Could we pack style updates in the same array with special keys or would that be too complicated?

Interestingly RN did this by packing all fields into one payload object. Caused a bunch of weird artifacts that made diffing harder to optimize.

We should reevaluate the actual algorithms here but for now I just wanted compatibility with CSSPropertyOperations since it is shared with Stack.

gaearon · 2017-01-12T00:19:02Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      } else if (registrationNameModules.hasOwnProperty(propKey)) {
+        if (nextProp) {
+          // We eagerly listen to this even though we haven't committed yet.
+          ensureListeningTo(rootContainerElement, propKey);


Why do we listen here instead of the commit?

One of the things this fixes is that getRootHostContainer is no longer needed in the commit phase if we do it here. As we know, getRootHostContainer is not safe to use during commits since we may not have the right context.

There's also no need to do this in the commit phase so might as well do it during the time sliced parts.

gaearon · 2017-01-12T00:24:26Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+  isCustomComponentTag : boolean,
+) : void {
+  // TODO: Handle wasCustomComponentTag
+  for (var i = 0; i < updatePayload.length; i+=2) {


Style nit: i += 2

gaearon · 2017-01-12T00:33:13Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+        }
+      } else if (propKey === DANGEROUSLY_SET_INNER_HTML ||
+                 propKey === CHILDREN) {
+        // TODO: Clear innerHTML. This is currently broken in Fiber because we are


I think @acdlite said this was solved.

gaearon · 2017-01-12T00:34:50Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      } else if (registrationNameModules.hasOwnProperty(propKey)) {
+        // This is a special case. If any listener updates we need to ensure
+        // that the "current" fiber pointer gets updated so we need a commit
+        // to update this element.


This depends on precacheFiber call in ReactDOMFiber.js, right? It's a bit non-obvious that an empty array here is needed to trigger the path there even though updateProperties in this file won't do anything. I guess doesn’t matter if we’ll unify them soon.

Also note that there is an open TODO items related to this.

gaearon · 2017-01-12T00:35:52Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+    for (propKey in nextProps) {
+      var nextProp = nextProps[propKey];
+      var lastProp =
+        lastProps != null ? lastProps[propKey] : undefined;


I don't think lastProps can be null here. You can probably read directly now that mount and update is separate.

Nice catch. I wish Flow would warn about this.

gaearon · 2017-01-12T00:39:48Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+        (updatePayload = updatePayload || []).push(propKey, null);
+      }
+    }
+    for (propKey in nextProps) {


While unrelated to this PR, I think the most common path is lastProps and nextProps with the same prop keys in the same order. I wonder if we could optimize this as a faster case with just one loop, and only do the second loop if we find a mismatch during the first one.

That assumes that you can iterate both interleaved which for...in doesn't let you do. It would require something like Object.keys or a generator with for...in to be really fast. Not too worried about this though. Something we can micro-optimize easily if needed.

gaearon · 2017-01-12T00:41:08Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+            }
+          }
+        } else {
+          // Relies on `updateStylesByID` not mutating `styleUpdates`.


Stray comment.

gaearon · 2017-01-12T00:50:53Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+          }
+        } else {
+          // Relies on `updateStylesByID` not mutating `styleUpdates`.
+          if (!styleUpdates) {


How can styleUpdates be truthy here? It could only get created if lastStyle existed and we wanted to delete some styles. But in this case we would get into if (lastProp) branch instead of this one. Unless I’m missing something.

The code in this branch is also wrong since it'll push a null. I'll delete.

gaearon · 2017-01-12T00:53:29Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      }
+    }
+    if (styleUpdates) {
+      (updatePayload = updatePayload || []).push(STYLE, styleUpdates);


Is it still necessary to delay it until the end? Why? This structure makes the above style code a little hard to follow, especially the case where you have to reassign styleUpdates right after pushing it to the payload so that it doesn't get skipped when it's falsy.

Donno. I just did a line-by-line translation.

If I don't put it here, I need it in all the places where I could create styleUpdates. Because there may not be a STYLE field in the nextProps but might be one in lastProps or vice versa. I also don't want to end up pushing it twice. I'm not sure that's better.

sebmarkbage · 2017-01-17T20:49:18Z

Ok, this I fixed the outstanding TODO by storing the "current" props on the DOM node and in the React Native instance cache. Not too happy about how this all came together but not really sure what we should do about the event system.

This is nice because it minimizes work during commit and initial mount but still relies on "diffing" event listeners during updates to figure out if we should update at all.

A plausible alternative model would be to traverse the "current" tree while processing the event, or even process events as part of reconciliation which does that traversal anyway.

acdlite · 2017-01-18T02:49:25Z

src/renderers/shared/fiber/ReactFiberCompleteWork.js

+          workInProgress.updateQueue = (updatePayload : any);
+          // If the update payload indicates that there is a change or if there
+          // is a new ref we mark this as an update.
+          if (updatePayload || current.ref !== workInProgress.ref) {


Refs shouldn't schedule an Update effect anymore, since we added the Ref effect

acdlite

👍

gaearon · 2017-01-18T06:11:02Z

scripts/fiber/tests-failing.txt

@@ -37,6 +37,9 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js
 * should throw on full document render w/ no markup
 * supports findDOMNode on full-page components

+src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
+* should control a value in reentrant events


I missed why this is failing.

Ah yes. I missed this too. This is probably either a bug or behavior change regarding event timing.

Fixed: #8839

This then gets stored on updateQueue so that the renderer doesn't need to think about how to store this. It then gets passed into commitUpdate during the commit phase. This allows renderers to do the diffing during the time sliced path, allocate a queue for changes and do only the absolute minimal work to apply those changes in the commit phase. If refs update we still schedule and update.

We now take advantage of the new capability to diff properties early. We do this by generating an update payload in the form of an array with each property name and value that we're about to update.

Wrapper components have custom logic that gets applied at the commit phase so we always need to ensure that we schedule an update for them.

No use case yet and I removed it from commitUpdate earlier.

This illustrates the problem that happens if we store a pointer to the Fiber and then choose not to update that pointer when no properties change. That causes an old Fiber to be retained on the DOM node. Then that Fiber can be reused by the pooling mechanism which then will mutate that Fiber with new event handlers, which makes them active before commit.

This represents the current set of event listeners. By not relying on the Fiber, it allows us to avoid doing any effects in the commit phase when nothing changes. This is a bit ugly. Not super happy how this all came together.

…rrentPropsFromNode - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget - After checking git blame for getFiberCurrentPropsFromNode and reading through facebook#8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.

…11383) * Rewrite ReactDOMComponentTree-test to test behavior using Public API - Part of #11299 - I've tried to identify cases where code within ReactDOMComponentTree is exercised and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments. * Prettier and lint changes * Remove testing of internals and add test cases for testing behavior exhibited after use of getInstanceFromNode * [RFC] Update testing approach to verify exhibited behavior dependent upon methods in ReactDOMComponentTree * Remove tests from event handlers and use sync tests * Prettier changes * Rename variables to be more semantic * Prettier updates * Update test following review - Use beforeEach and afterEach to set up and tear down container element for use in each test - Move any functions specific to one test to within test body (improves readability imo) * Add coverage for getNodeFromInstance and implementation of getFiberCurrentPropsFromNode - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget - After checking git blame for getFiberCurrentPropsFromNode and reading through #8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.

…acebook#11383) * Rewrite ReactDOMComponentTree-test to test behavior using Public API - Part of facebook#11299 - I've tried to identify cases where code within ReactDOMComponentTree is exercised and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments. * Prettier and lint changes * Remove testing of internals and add test cases for testing behavior exhibited after use of getInstanceFromNode * [RFC] Update testing approach to verify exhibited behavior dependent upon methods in ReactDOMComponentTree * Remove tests from event handlers and use sync tests * Prettier changes * Rename variables to be more semantic * Prettier updates * Update test following review - Use beforeEach and afterEach to set up and tear down container element for use in each test - Move any functions specific to one test to within test body (improves readability imo) * Add coverage for getNodeFromInstance and implementation of getFiberCurrentPropsFromNode - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget - After checking git blame for getFiberCurrentPropsFromNode and reading through facebook#8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.

sebmarkbage assigned sophiebits and gaearon Dec 20, 2016

sebmarkbage requested review from sophiebits and gaearon December 20, 2016 03:32

facebook-github-bot added the GH Review: review-needed label Dec 20, 2016

sebmarkbage mentioned this pull request Dec 20, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sebmarkbage assigned bvaughn and acdlite Dec 20, 2016

sebmarkbage commented Dec 20, 2016

View reviewed changes

sebmarkbage force-pushed the fiberpreparepayload branch from 8eff6da to 7b2e66a Compare December 20, 2016 21:08

facebook-github-bot added the CLA Signed label Dec 29, 2016

gaearon reviewed Jan 11, 2017

View reviewed changes

sebmarkbage mentioned this pull request Jan 11, 2017

Make callbackList a linked-list instead of an array #8752

Closed

sebmarkbage force-pushed the fiberpreparepayload branch from 7b2e66a to f22de36 Compare January 11, 2017 21:38

gaearon reviewed Jan 12, 2017

View reviewed changes

sebmarkbage changed the title ~~[WIP][Fiber] Compute the Host Diff During Reconciliation~~ [Fiber] Compute the Host Diff During Reconciliation Jan 17, 2017

sebmarkbage force-pushed the fiberpreparepayload branch from 95689ce to 43fcc24 Compare January 17, 2017 20:44

acdlite reviewed Jan 18, 2017

View reviewed changes

acdlite approved these changes Jan 18, 2017

View reviewed changes

gaearon reviewed Jan 18, 2017

View reviewed changes

sebmarkbage added 10 commits January 19, 2017 21:01

Diff ReactDOMFiber properties in prepareUpdate

79df93d

We now take advantage of the new capability to diff properties early. We do this by generating an update payload in the form of an array with each property name and value that we're about to update.

Hack around the listener problem

b46ff41

Add todo for handling wasCustomComponentTag

2017b3a

Always force an update to wrapper components

c729a95

Wrapper components have custom logic that gets applied at the commit phase so we always need to ensure that we schedule an update for them.

Remove rootContainerInstance from commitMount

0be90df

No use case yet and I removed it from commitUpdate earlier.

Use update signal object in test renderer

797cf96

Incorporate 8652 into new algorithm

cad6028

Fix comment

29be1fa

sebmarkbage force-pushed the fiberpreparepayload branch from 43fcc24 to ef651e8 Compare January 20, 2017 05:03

sebmarkbage merged commit b354db2 into facebook:master Jan 20, 2017

sebmarkbage mentioned this pull request Jan 21, 2017

Read "current" props from the node instead of the Fiber #8839

Merged

iamdustan mentioned this pull request Sep 28, 2017

return value type in prepareUpdate #10904

Closed

GordyD mentioned this pull request Nov 19, 2017

Rewrite ReactDOMComponentTree-test to test behavior using Public API #11383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Compute the Host Diff During Reconciliation #8607

[Fiber] Compute the Host Diff During Reconciliation #8607

sebmarkbage commented Dec 20, 2016 •

edited

Loading

sebmarkbage Dec 20, 2016

bvaughn Dec 20, 2016

gaearon Jan 11, 2017

gaearon commented Jan 11, 2017

sebmarkbage commented Jan 11, 2017

gaearon commented Jan 11, 2017

gaearon commented Jan 12, 2017

sebmarkbage commented Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

gaearon Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

gaearon Jan 12, 2017

sebmarkbage Jan 12, 2017

sebmarkbage Jan 12, 2017 •

edited

Loading

sebmarkbage commented Jan 17, 2017

acdlite Jan 18, 2017

acdlite left a comment

gaearon Jan 18, 2017

sebmarkbage Jan 21, 2017

sebmarkbage Jan 21, 2017

[Fiber] Compute the Host Diff During Reconciliation #8607

[Fiber] Compute the Host Diff During Reconciliation #8607

Conversation

sebmarkbage commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jan 11, 2017

sebmarkbage commented Jan 11, 2017

gaearon commented Jan 11, 2017

gaearon commented Jan 12, 2017

sebmarkbage commented Jan 12, 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

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

sebmarkbage Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

sebmarkbage commented Jan 17, 2017

Choose a reason for hiding this comment

acdlite 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

sebmarkbage commented Dec 20, 2016 •

edited

Loading

sebmarkbage Jan 12, 2017 •

edited

Loading