From b354db22a9a3ecf79945a618cf97bed688a9364c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 19 Jan 2017 21:56:15 -0800 Subject: [PATCH] [Fiber] Compute the Host Diff During Reconciliation (#8607) * Allow renderers to return an update payload in prepareUpdate 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. * Hack around the listener problem * Diff ReactDOMFiber properties in prepareUpdate 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. * Add todo for handling wasCustomComponentTag * Always force an update to wrapper components 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 No use case yet and I removed it from commitUpdate earlier. * Use update signal object in test renderer * Incorporate 8652 into new algorithm * Fix comment * Add failing test for flipping event handlers 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. * Store current props in the RN instance cache and on the DOM node 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. --- scripts/fiber/tests-failing.txt | 3 + scripts/fiber/tests-passing.txt | 2 +- src/renderers/art/ReactARTFiber.js | 6 +- src/renderers/dom/fiber/ReactDOMFiber.js | 24 +- .../dom/fiber/ReactDOMFiberComponent.js | 311 ++++++++++++------ .../dom/fiber/__tests__/ReactDOMFiber-test.js | 83 +++++ .../dom/shared/ReactDOMComponentTree.js | 17 +- .../native/ReactNativeComponentTree.js | 11 + src/renderers/native/ReactNativeFiber.js | 28 +- src/renderers/noop/ReactNoop.js | 8 +- .../shared/fiber/ReactFiberBeginWork.js | 4 +- .../shared/fiber/ReactFiberCommitWork.js | 21 +- .../shared/fiber/ReactFiberCompleteWork.js | 20 +- .../shared/fiber/ReactFiberHostContext.js | 4 +- .../shared/fiber/ReactFiberReconciler.js | 10 +- .../shared/fiber/ReactFiberScheduler.js | 4 +- .../shared/shared/ReactTreeTraversal.js | 8 +- .../shared/shared/event/EventPluginHub.js | 8 +- .../shared/shared/event/EventPluginUtils.js | 3 + .../testing/ReactTestRendererFiber.js | 10 +- 20 files changed, 412 insertions(+), 173 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 884b8c3dc5..fdde0f21ec 100644 --- a/scripts/fiber/tests-failing.txt +++ b/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 + src/renderers/shared/__tests__/ReactPerf-test.js * should count no-op update as waste * should count no-op update in child as waste diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index e1415a100a..21c9842716 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -546,6 +546,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * findDOMNode should find dom element after expanding a fragment * should bubble events from the portal to the parent * should not onMouseLeave when staying in the portal +* should not update event handlers until commit * should not crash encountering low-priority tree * throws if non-element passed to top-level render * throws if something other than false, null, or an element is returned from render @@ -959,7 +960,6 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMIframe-test.js src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should properly control a value even if no event listener exists -* should control a value in reentrant events * should control values in reentrant events with different targets * should display `defaultValue` of number 0 * only assigns defaultValue if it changes diff --git a/src/renderers/art/ReactARTFiber.js b/src/renderers/art/ReactARTFiber.js index ee9aa7d859..fcdcc957e0 100644 --- a/src/renderers/art/ReactARTFiber.js +++ b/src/renderers/art/ReactARTFiber.js @@ -42,6 +42,8 @@ const TYPES = { TEXT: 'Text', }; +const UPDATE_SIGNAL = {}; + /** Helper Methods */ function addEventListeners(instance, type, listener) { @@ -418,7 +420,7 @@ const ARTRenderer = ReactFiberReconciler({ // Noop }, - commitUpdate(instance, type, oldProps, newProps) { + commitUpdate(instance, updatePayload, type, oldProps, newProps) { instance._applyProps(instance, newProps, oldProps); }, @@ -482,7 +484,7 @@ const ARTRenderer = ReactFiberReconciler({ }, prepareUpdate(domElement, type, oldProps, newProps) { - return true; + return UPDATE_SIGNAL; }, removeChild(parentInstance, child) { diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index fc7cb30e83..487013cbc1 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -35,9 +35,13 @@ var { createElement, getChildNamespace, setInitialProperties, + diffProperties, updateProperties, } = ReactDOMFiberComponent; -var { precacheFiberNode } = ReactDOMComponentTree; +var { + precacheFiberNode, + updateFiberEventHandlers, +} = ReactDOMComponentTree; if (__DEV__) { var validateDOMNesting = require('validateDOMNesting'); @@ -184,6 +188,7 @@ var DOMRenderer = ReactFiberReconciler({ } const domElement : Instance = createElement(type, props, rootContainerInstance, parentNamespace); precacheFiberNode(internalInstanceHandle, domElement); + updateFiberEventHandlers(domElement, props); return domElement; }, @@ -206,8 +211,9 @@ var DOMRenderer = ReactFiberReconciler({ type : string, oldProps : Props, newProps : Props, + rootContainerInstance : Container, hostContext : HostContext, - ) : boolean { + ) : null | Array { if (__DEV__) { const hostContextDev = ((hostContext : any) : HostContextDev); if (typeof newProps.children !== typeof oldProps.children && ( @@ -218,14 +224,13 @@ var DOMRenderer = ReactFiberReconciler({ validateDOMNesting(null, String(newProps.children), null, ownAncestorInfo); } } - return true; + return diffProperties(domElement, type, oldProps, newProps, rootContainerInstance); }, commitMount( domElement : Instance, type : string, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { ((domElement : any) : HTMLButtonElement | HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement).focus(); @@ -233,16 +238,17 @@ var DOMRenderer = ReactFiberReconciler({ commitUpdate( domElement : Instance, + updatePayload : Array, type : string, oldProps : Props, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { - // Update the internal instance handle so that we know which props are - // the current ones. - precacheFiberNode(internalInstanceHandle, domElement); - updateProperties(domElement, type, oldProps, newProps, rootContainerInstance); + // Update the props handle so that we know which props are the ones with + // with current event handlers. + updateFiberEventHandlers(domElement, newProps); + // Apply the diff to the DOM node. + updateProperties(domElement, updatePayload, type, oldProps, newProps); }, shouldSetTextContent(props : Props) : boolean { diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index f75c0ad9eb..58d0a166d2 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -301,69 +301,15 @@ function isCustomComponent(tagName, props) { return tagName.indexOf('-') >= 0 || props.is != null; } -/** - * Reconciles the properties by detecting differences in property values and - * updating the DOM as necessary. This function is probably the single most - * critical path for performance optimization. - * - * TODO: Benchmark whether checking for changed values in memory actually - * improves performance (especially statically positioned elements). - * TODO: Benchmark the effects of putting this at the top since 99% of props - * do not change for a given reconciliation. - * TODO: Benchmark areas that can be improved with caching. - */ -function updateDOMProperties( +function setInitialDOMProperties( domElement : Element, rootContainerElement : Element, - lastProps : null | Object, nextProps : Object, - wasCustomComponentTag : boolean, isCustomComponentTag : boolean, ) : void { - var propKey; - var styleName; - var styleUpdates; - for (propKey in lastProps) { - if (nextProps.hasOwnProperty(propKey) || - !lastProps.hasOwnProperty(propKey) || - lastProps[propKey] == null) { - continue; - } - if (propKey === STYLE) { - var lastStyle = lastProps[propKey]; - for (styleName in lastStyle) { - if (lastStyle.hasOwnProperty(styleName)) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - } else if (propKey === DANGEROUSLY_SET_INNER_HTML || - propKey === CHILDREN) { - // TODO: Clear innerHTML. This is currently broken in Fiber because we are - // too late to clear everything at this point because new children have - // already been inserted. - } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { - // Noop - } else if (registrationNameModules.hasOwnProperty(propKey)) { - // Do nothing for deleted listeners. - } else if (wasCustomComponentTag) { - DOMPropertyOperations.deleteValueForAttribute( - domElement, - propKey - ); - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey)) { - DOMPropertyOperations.deleteValueForProperty(domElement, propKey); - } - } - for (propKey in nextProps) { + for (var propKey in nextProps) { var nextProp = nextProps[propKey]; - var lastProp = - lastProps != null ? lastProps[propKey] : undefined; - if (!nextProps.hasOwnProperty(propKey) || - nextProp === lastProp || - nextProp == null && lastProp == null) { + if (!nextProps.hasOwnProperty(propKey)) { continue; } if (propKey === STYLE) { @@ -374,38 +320,16 @@ function updateDOMProperties( Object.freeze(nextProp); } } - if (lastProp) { - // Unset styles on `lastProp` but not on `nextProp`. - for (styleName in lastProp) { - if (lastProp.hasOwnProperty(styleName) && - (!nextProp || !nextProp.hasOwnProperty(styleName))) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - // Update styles that changed since `lastProp`. - for (styleName in nextProp) { - if (nextProp.hasOwnProperty(styleName) && - lastProp[styleName] !== nextProp[styleName]) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = nextProp[styleName]; - } - } - } else { - // Relies on `updateStylesByID` not mutating `styleUpdates`. - styleUpdates = nextProp; - } + // Relies on `updateStylesByID` not mutating `styleUpdates`. + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + nextProp, + ); } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { var nextHtml = nextProp ? nextProp[HTML] : undefined; - var lastHtml = lastProp ? lastProp[HTML] : undefined; - // Intentional use of != to avoid catching zero/false. if (nextHtml != null) { - if (lastHtml !== nextHtml) { - setInnerHTML(domElement, '' + nextHtml); - } - } else { - // TODO: It might be too late to clear this if we have children - // inserted already. + setInnerHTML(domElement, nextHtml); } } else if (propKey === CHILDREN) { if (typeof nextProp === 'string') { @@ -433,18 +357,57 @@ function updateDOMProperties( // brings us in line with the same behavior we have on initial render. if (nextProp != null) { DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp); + } + } + } +} + +function updateDOMProperties( + domElement : Element, + updatePayload : Array, + wasCustomComponentTag : boolean, + isCustomComponentTag : boolean, +) : void { + // TODO: Handle wasCustomComponentTag + for (var i = 0; i < updatePayload.length; i+=2) { + var propKey = updatePayload[i]; + var propValue = updatePayload[i + 1]; + if (propKey === STYLE) { + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + propValue, + ); + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + setInnerHTML(domElement, propValue); + } else if (propKey === CHILDREN) { + setTextContent(domElement, propValue); + } else if (isCustomComponentTag) { + if (propValue != null) { + DOMPropertyOperations.setValueForAttribute( + domElement, + propKey, + propValue + ); + } else { + DOMPropertyOperations.deleteValueForAttribute( + domElement, + propKey + ); + } + } else if ( + DOMProperty.properties[propKey] || + DOMProperty.isCustomAttribute(propKey)) { + // If we're updating to null or undefined, we should remove the property + // from the DOM node instead of inadvertently setting to a string. This + // brings us in line with the same behavior we have on initial render. + if (propValue != null) { + DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue); } else { DOMPropertyOperations.deleteValueForProperty(domElement, propKey); } } } - if (styleUpdates) { - // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. - CSSPropertyOperations.setValueForStyles( - domElement, - styleUpdates, - ); - } } // Assumes there is no parent namespace. @@ -592,12 +555,10 @@ var ReactDOMFiberComponent = { assertValidProps(tag, props); - updateDOMProperties( + setInitialDOMProperties( domElement, rootContainerElement, - null, props, - false, isCustomComponentTag ); @@ -626,35 +587,42 @@ var ReactDOMFiberComponent = { } }, - updateProperties( + // Calculate the diff between the two objects. + diffProperties( domElement : Element, tag : string, lastRawProps : Object, nextRawProps : Object, rootContainerElement : Element, - ) : void { + ) : null | Array { if (__DEV__) { validatePropertiesInDevelopment(tag, nextRawProps); } + var updatePayload : null | Array = null; + var lastProps : Object; var nextProps : Object; switch (tag) { case 'input': lastProps = ReactDOMFiberInput.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberInput.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'option': lastProps = ReactDOMFiberOption.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberOption.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'select': lastProps = ReactDOMFiberSelect.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberSelect.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'textarea': lastProps = ReactDOMFiberTextarea.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberTextarea.getHostProps(domElement, nextRawProps); + updatePayload = []; break; default: lastProps = lastRawProps; @@ -668,17 +636,154 @@ var ReactDOMFiberComponent = { } assertValidProps(tag, nextProps); - var wasCustomComponentTag = isCustomComponent(tag, lastProps); - var isCustomComponentTag = isCustomComponent(tag, nextProps); + + var propKey; + var styleName; + var styleUpdates = null; + for (propKey in lastProps) { + if (nextProps.hasOwnProperty(propKey) || + !lastProps.hasOwnProperty(propKey) || + lastProps[propKey] == null) { + continue; + } + if (propKey === STYLE) { + var lastStyle = lastProps[propKey]; + for (styleName in lastStyle) { + if (lastStyle.hasOwnProperty(styleName)) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML || + propKey === CHILDREN) { + // Noop. This is handled by the clear text mechanism. + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } 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. + if (!updatePayload) { + updatePayload = []; + } + } else { + // For all other deleted properties we add it to the queue. We use + // the whitelist in the commit phase instead. + (updatePayload = updatePayload || []).push(propKey, null); + } + } + for (propKey in nextProps) { + var nextProp = nextProps[propKey]; + var lastProp = + lastProps != null ? lastProps[propKey] : undefined; + if (!nextProps.hasOwnProperty(propKey) || + nextProp === lastProp || + nextProp == null && lastProp == null) { + continue; + } + if (propKey === STYLE) { + if (__DEV__) { + if (nextProp) { + // Freeze the next style object so that we can assume it won't be + // mutated. We have already warned for this in the past. + Object.freeze(nextProp); + } + } + if (lastProp) { + // Unset styles on `lastProp` but not on `nextProp`. + for (styleName in lastProp) { + if (lastProp.hasOwnProperty(styleName) && + (!nextProp || !nextProp.hasOwnProperty(styleName))) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + // Update styles that changed since `lastProp`. + for (styleName in nextProp) { + if (nextProp.hasOwnProperty(styleName) && + lastProp[styleName] !== nextProp[styleName]) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = nextProp[styleName]; + } + } + } else { + // Relies on `updateStylesByID` not mutating `styleUpdates`. + if (!styleUpdates) { + if (!updatePayload) { + updatePayload = []; + } + updatePayload.push(propKey, styleUpdates); + } + styleUpdates = nextProp; + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + var nextHtml = nextProp ? nextProp[HTML] : undefined; + var lastHtml = lastProp ? lastProp[HTML] : undefined; + if (nextHtml != null) { + if (lastHtml !== nextHtml) { + (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); + } + } else { + // TODO: It might be too late to clear this if we have children + // inserted already. + } + } else if (propKey === CHILDREN) { + if (lastProp !== nextProp && ( + typeof nextProp === 'string' || typeof nextProp === 'number' + )) { + (updatePayload = updatePayload || []).push(propKey, '' + nextProp); + } + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } else if (registrationNameModules.hasOwnProperty(propKey)) { + if (nextProp) { + // We eagerly listen to this even though we haven't committed yet. + ensureListeningTo(rootContainerElement, propKey); + } + if (!updatePayload && lastProp !== nextProp) { + // This is a special case. If any listener updates we need to ensure + // that the "current" props pointer gets updated so we need a commit + // to update this element. + updatePayload = []; + } + } else { + // For any other property we always add it to the queue and then we + // filter it out using the whitelist during the commit. + (updatePayload = updatePayload || []).push(propKey, nextProp); + } + } + if (styleUpdates) { + (updatePayload = updatePayload || []).push(STYLE, styleUpdates); + } + return updatePayload; + }, + + // Apply the diff. + updateProperties( + domElement : Element, + updatePayload : Array, + tag : string, + lastRawProps : Object, + nextRawProps : Object + ) : void { + var wasCustomComponentTag = isCustomComponent(tag, lastRawProps); + var isCustomComponentTag = isCustomComponent(tag, nextRawProps); + // Apply the diff. updateDOMProperties( domElement, - rootContainerElement, - lastProps, - nextProps, + updatePayload, wasCustomComponentTag, isCustomComponentTag ); + // TODO: Ensure that an update gets scheduled if any of the special props + // changed. switch (tag) { case 'input': // Update the wrapper around inputs *after* updating props. This has to diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index ea7f39da29..41c7f129b7 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -994,6 +994,89 @@ describe('ReactDOMFiber', () => { ]); }); + it('should not update event handlers until commit', () => { + let ops = []; + const handlerA = () => ops.push('A'); + const handlerB = () => ops.push('B'); + + class Example extends React.Component { + state = { flip: false, count: 0 }; + flip() { + this.setState({ flip: true, count: this.state.count + 1 }); + } + tick() { + this.setState({ count: this.state.count + 1 }); + } + render() { + const useB = !this.props.forceA && this.state.flip; + return ( +
+ ); + } + } + + class Click extends React.Component { + constructor() { + super(); + click(node); + } + render() { + return null; + } + } + + let inst; + ReactDOM.render([ inst = n} />], container); + const node = container.firstChild; + expect(node.tagName).toEqual('DIV'); + + function click(target) { + var fakeNativeEvent = {}; + ReactTestUtils.simulateNativeEventOnNode( + 'topClick', + target, + fakeNativeEvent + ); + } + + click(node); + + expect(ops).toEqual(['A']); + ops = []; + + // Render with the other event handler. + inst.flip(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Rerender without changing any props. + inst.tick(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Render a flip back to the A handler. The second component invokes the + // click handler during render to simulate a click during an aborted + // render. I use this hack because at current time we don't have a way to + // test aborted ReactDOM renders. + ReactDOM.render([, ], container); + + // Because the new click handler has not yet committed, we should still + // invoke B. + expect(ops).toEqual(['B']); + ops = []; + + // Any click that happens after commit, should invoke A. + click(node); + expect(ops).toEqual(['A']); + + }); + it('should not crash encountering low-priority tree', () => { ReactDOM.render(