Diff properties in the commit phase instead of generating an update payload #26583

sebmarkbage · 2023-04-10T03:49:34Z

This removes the concept of prepareUpdate(), behind a flag.

React Native already does everything in the commit phase, but generates a temporary update payload before applying it.

React Fabric does it both in the render phase. Now it just moves it to a single host config.

For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload.

There are a few downsides of this approach:

If only "children" has changed, we end up scheduling an update to be done in the commit phase. Since we traverse through it anyway, it's probably not much extra.
It does more work in the commit phase so for a large tree that is mostly unchanged, it'll stall longer.
It does some extra work for special cases since that work happens if anything has changed. We no longer have a deep bailout.
The special cases now have to each replicate the "clean up old props" loop, leading to extra code.

The benefit is that this doesn't allocate temporary extra objects (possibly multiple per element if the array has to resize). It's less work overall. It also gives us an option to reuse this function for a sync render optimization.

Another benefit is that if we do the loop in the commit phase I can do further optimizations by reading all props that I need for special cases in that loop instead of polymorphic reads from props. This is what I'd like to do in future refactors that would be stacked on top of this change.

sebmarkbage · 2023-04-10T03:52:01Z

packages/shared/forks/ReactFeatureFlags.www-dynamic.js

@@ -25,6 +25,7 @@ export const enableUnifiedSyncLane = __VARIANT__;
 export const enableTransitionTracing = __VARIANT__;
 export const enableCustomElementPropertySupport = __VARIANT__;
 export const enableDeferRootSchedulingToMicrotask = __VARIANT__;
+export const diffInCommitPhase = __VARIANT__;


@kassens I prepped this for www since it would be good to test that this doesn't regress asap.

react-sizebot · 2023-04-10T03:56:44Z

Comparing: dd0619b...d74688c

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.02%	164.58 kB	164.61 kB	+0.02%	51.68 kB	51.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.51%	166.15 kB	167.00 kB	+0.21%	52.20 kB	52.31 kB
facebook-www/ReactDOM-prod.classic.js	+2.02%	554.05 kB	565.24 kB	+1.19%	98.17 kB	99.34 kB
facebook-www/ReactDOM-prod.modern.js	+2.08%	537.89 kB	549.08 kB	+1.21%	95.51 kB	96.66 kB
facebook-www/ReactDOMTesting-prod.modern.js	+2.02%	554.43 kB	565.62 kB	+1.17%	99.68 kB	100.84 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOM-prod.modern.js	+2.08%	537.89 kB	549.08 kB	+1.21%	95.51 kB	96.66 kB
facebook-www/ReactDOM-prod.classic.js	+2.02%	554.05 kB	565.24 kB	+1.19%	98.17 kB	99.34 kB
facebook-www/ReactDOMTesting-prod.modern.js	+2.02%	554.43 kB	565.62 kB	+1.17%	99.68 kB	100.84 kB
facebook-www/ReactDOM-profiling.modern.js	+1.97%	568.33 kB	579.50 kB	+1.14%	100.01 kB	101.15 kB
facebook-www/ReactDOM-profiling.classic.js	+1.91%	584.55 kB	595.72 kB	+1.12%	102.73 kB	103.88 kB
facebook-www/ReactDOMTesting-prod.classic.js	+1.89%	568.75 kB	579.50 kB	+1.16%	101.83 kB	103.02 kB
facebook-www/ReactDOM-dev.modern.js	+1.24%	1,378.66 kB	1,395.81 kB	+0.71%	299.12 kB	301.23 kB
facebook-www/ReactDOMTesting-dev.modern.js	+1.23%	1,397.06 kB	1,414.21 kB	+0.69%	303.62 kB	305.71 kB
facebook-www/ReactDOM-dev.classic.js	+1.22%	1,406.46 kB	1,423.62 kB	+0.68%	304.71 kB	306.80 kB
facebook-www/ReactDOMTesting-dev.classic.js	+1.20%	1,424.86 kB	1,442.01 kB	+0.69%	308.91 kB	311.04 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.51%	166.15 kB	167.00 kB	+0.21%	52.20 kB	52.31 kB
oss-experimental/react-dom/umd/react-dom.production.min.js	+0.50%	166.07 kB	166.91 kB	+0.11%	52.62 kB	52.68 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.min.js	+0.49%	172.37 kB	173.21 kB	+0.19%	54.53 kB	54.63 kB
oss-experimental/react-dom/cjs/react-dom.profiling.min.js	+0.48%	175.78 kB	176.63 kB	+0.18%	54.63 kB	54.73 kB
oss-experimental/react-dom/umd/react-dom.profiling.min.js	+0.48%	175.05 kB	175.89 kB	+0.13%	54.92 kB	54.99 kB
oss-experimental/react-dom/cjs/react-dom.development.js	+0.24%	1,264.45 kB	1,267.47 kB	+0.11%	278.59 kB	278.89 kB
oss-experimental/react-dom/umd/react-dom.development.js	+0.24%	1,325.77 kB	1,328.92 kB	+0.11%	281.38 kB	281.69 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js	+0.24%	1,282.56 kB	1,285.58 kB	+0.10%	283.02 kB	283.29 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.91 kB	42.62 kB	=	9.64 kB	9.61 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.91 kB	42.62 kB	=	9.64 kB	9.61 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.91 kB	42.62 kB	=	9.64 kB	9.61 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.78 kB	42.49 kB	=	9.63 kB	9.60 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.78 kB	42.49 kB	=	9.63 kB	9.60 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.78 kB	42.49 kB	=	9.63 kB	9.60 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	=	15.76 kB	15.64 kB	=	4.69 kB	4.67 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	=	15.76 kB	15.64 kB	=	4.69 kB	4.67 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	=	15.76 kB	15.64 kB	=	4.69 kB	4.67 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.production.min.js	=	15.69 kB	15.57 kB	=	4.68 kB	4.66 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.production.min.js	=	15.69 kB	15.57 kB	=	4.68 kB	4.66 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.production.min.js	=	15.69 kB	15.57 kB	=	4.68 kB	4.66 kB

Generated by 🚫 dangerJS against d74688c

sebmarkbage · 2023-04-10T04:16:40Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

@@ -273,6 +275,7 @@ function setProp(
  key: string,
  value: mixed,
  props: any,
+  prevValue: mixed,


The props argument is only used by dangerouslySetInnerHTML to check that there's not also children. prevValue is only used by style. A possible alternative would be to special case these two in each loop instead.

packages/react-dom-bindings/src/client/CSSPropertyOperations.js

sebmarkbage · 2023-04-10T19:56:21Z

There are two things not implemented in the new flag. Neither are covered by tests.

This DEV warning which only affects updating to start using a single text child, in a parent where that's not valid. The tricky part is that we don't have the host context anymore in the commit phase.

react/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js

Lines 488 to 502 in 0ae3480

    
           if (__DEV__) { 
        
             const hostContextDev = ((hostContext: any): HostContextDev); 
        
             if ( 
        
               typeof newProps.children !== typeof oldProps.children && 
        
               (typeof newProps.children === 'string' || 
        
                 typeof newProps.children === 'number') 
        
             ) { 
        
               const string = '' + newProps.children; 
        
               const ownAncestorInfo = updatedAncestorInfoDev( 
        
                 hostContextDev.ancestorInfo, 
        
                 type, 
        
               ); 
        
               validateDOMNesting(null, string, ownAncestorInfo); 
        
             } 
        
           }

~~The is warning I recently added. Could probably add this in setProp and just have it compiled out.~~ Fixed.

react/packages/react-dom-bindings/src/client/ReactDOMComponent.js

Lines 773 to 777 in 0ae3480

    
           if (__DEV__) { 
        
             console.error( 
        
               'Cannot update the "is" prop after it has been initialized.', 
        
             ); 
        
           }

sophiebits

the duplication in the clean-up-lastProps loops is kinda annoying. you don't want to extract each loop body to one function per tag? I'd think that would be fast

sophiebits · 2023-04-10T21:12:08Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

@@ -273,6 +274,7 @@ function setProp(
  key: string,
  value: mixed,
  props: any,
+  prevValue: mixed,
 ): void {
  switch (key) {
    case 'children': {


Move the validateDOMNesting single text child logic here? I don't think it needs the ancestors for that check, just the parent tag name.

I'll do that separately. Seems like I should break it out of the validateDOMNesting helper into separate one that doesn't require the full ancestor info.

sophiebits · 2023-04-10T21:21:41Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

+        if (
+          nextProps.hasOwnProperty(propKey) &&
+          nextProp !== lastProp &&
+          (nextProp != null || lastProp != null)


(Why don't we cover lastProp != null && nextProp == null in the prior loop?)

What do you mean?

(I change this in the upcoming PR anyway.)

When a prop transitions from present to null, the second loop handles it but it seems like maybe it would be simpler for the first loop to.

I think this case is basically just checking that we don't trigger an update if you transition from undefined to null or vice versa. Seems unnecessary tbh.

Most props have some non-null value that's the same as deleted now (like function/symbol) so it's not really much extra work also check for null there but maybe. setProp still needs to deal with it. I considered having a separate deleteProp instead of setting null but because it has to go through the special case route and each of those have some other way to delete already, it didn't make it worth it.

sophiebits · 2023-04-10T21:33:44Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

+      // Update checked *before* name.
+      // In the middle of an update, it is possible to have multiple checked.
+      // When a checked radio tries to change name, browser makes another radio's checked false.
+      if (nextProps.type === 'radio' && nextProps.name != null) {


this logic is insufficient even today #26588

packages/react-dom-bindings/src/client/ReactDOMComponent.js

sebmarkbage · 2023-04-10T22:10:32Z

the duplication in the clean-up-lastProps loops is kinda annoying. you don't want to extract each loop body to one function per tag? I'd think that would be fast

It'll be clear in my next PR why I don't want to do that. Because I keep track of the props I've read outside of the loop body so I need the same mutable stack to be available to me.

…ayload This removes the concept of prepareUpdate(). React Native already does everything in the commit phase, but generates a temporary update payload before applying it. React Fabric does it both in the render phase. Now it just moves it to a single host config. For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload.

Also fixes a bug where styles weren't cleared on custom elements.

…ayload (#26583) This removes the concept of `prepareUpdate()`, behind a flag. React Native already does everything in the commit phase, but generates a temporary update payload before applying it. React Fabric does it both in the render phase. Now it just moves it to a single host config. For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload. There are a few downsides of this approach: - If only "children" has changed, we end up scheduling an update to be done in the commit phase. Since we traverse through it anyway, it's probably not much extra. - It does more work in the commit phase so for a large tree that is mostly unchanged, it'll stall longer. - It does some extra work for special cases since that work happens if anything has changed. We no longer have a deep bailout. - The special cases now have to each replicate the "clean up old props" loop, leading to extra code. The benefit is that this doesn't allocate temporary extra objects (possibly multiple per element if the array has to resize). It's less work overall. It also gives us an option to reuse this function for a sync render optimization. Another benefit is that if we do the loop in the commit phase I can do further optimizations by reading all props that I need for special cases in that loop instead of polymorphic reads from props. This is what I'd like to do in future refactors that would be stacked on top of this change. DiffTrain build for [ca41adb](ca41adb)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

…update payload (facebook#26583)" This reverts commit ca41adb.

, facebook#26595, facebook#26596, facebook#26627 - Refactor some controlled component stuff (facebook#26573) - Don't update textarea defaultValue and input checked unnecessarily (facebook#26580) - Diff properties in the commit phase instead of generating an update payload (facebook#26583) - Move validation of text nesting into ReactDOMComponent (facebook#26594) - Remove initOption special case (facebook#26595) - Use already extracted values instead of reading off props for controlled components (facebook#26596) - Fix input tracking bug (facebook#26627)

- Refactor some controlled component stuff (#26573) - Don't update textarea defaultValue and input checked unnecessarily (#26580) - Diff properties in the commit phase instead of generating an update payload (#26583) - Move validation of text nesting into ReactDOMComponent (#26594) - Remove initOption special case (#26595) - Use already extracted values instead of reading off props for controlled components (#26596) - Fix input tracking bug (#26627) DiffTrain build for [ded4a78](ded4a78)

- Refactor some controlled component stuff (#26573) - Don't update textarea defaultValue and input checked unnecessarily (#26580) - Diff properties in the commit phase instead of generating an update payload (#26583) - Move validation of text nesting into ReactDOMComponent (#26594) - Remove initOption special case (#26595) - Use already extracted values instead of reading off props for controlled components (#26596) - Fix input tracking bug (#26627)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

…ayload (facebook#26583) This removes the concept of `prepareUpdate()`, behind a flag. React Native already does everything in the commit phase, but generates a temporary update payload before applying it. React Fabric does it both in the render phase. Now it just moves it to a single host config. For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload. There are a few downsides of this approach: - If only "children" has changed, we end up scheduling an update to be done in the commit phase. Since we traverse through it anyway, it's probably not much extra. - It does more work in the commit phase so for a large tree that is mostly unchanged, it'll stall longer. - It does some extra work for special cases since that work happens if anything has changed. We no longer have a deep bailout. - The special cases now have to each replicate the "clean up old props" loop, leading to extra code. The benefit is that this doesn't allocate temporary extra objects (possibly multiple per element if the array has to resize). It's less work overall. It also gives us an option to reuse this function for a sync render optimization. Another benefit is that if we do the loop in the commit phase I can do further optimizations by reading all props that I need for special cases in that loop instead of polymorphic reads from props. This is what I'd like to do in future refactors that would be stacked on top of this change.

…ayload (#26583) This removes the concept of `prepareUpdate()`, behind a flag. React Native already does everything in the commit phase, but generates a temporary update payload before applying it. React Fabric does it both in the render phase. Now it just moves it to a single host config. For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload. There are a few downsides of this approach: - If only "children" has changed, we end up scheduling an update to be done in the commit phase. Since we traverse through it anyway, it's probably not much extra. - It does more work in the commit phase so for a large tree that is mostly unchanged, it'll stall longer. - It does some extra work for special cases since that work happens if anything has changed. We no longer have a deep bailout. - The special cases now have to each replicate the "clean up old props" loop, leading to extra code. The benefit is that this doesn't allocate temporary extra objects (possibly multiple per element if the array has to resize). It's less work overall. It also gives us an option to reuse this function for a sync render optimization. Another benefit is that if we do the loop in the commit phase I can do further optimizations by reading all props that I need for special cases in that loop instead of polymorphic reads from props. This is what I'd like to do in future refactors that would be stacked on top of this change. DiffTrain build for commit ca41adb.

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 10, 2023

sebmarkbage requested review from sophiebits and acdlite April 10, 2023 03:49

sebmarkbage force-pushed the diffincommit branch from 18cae56 to e906f64 Compare April 10, 2023 03:50

sebmarkbage commented Apr 10, 2023

View reviewed changes

sebmarkbage force-pushed the diffincommit branch from e906f64 to 3344b75 Compare April 10, 2023 03:56

sebmarkbage force-pushed the diffincommit branch 2 times, most recently from 2186650 to e350d9b Compare April 10, 2023 04:12

sebmarkbage commented Apr 10, 2023

View reviewed changes

packages/react-dom-bindings/src/client/CSSPropertyOperations.js Show resolved Hide resolved

sophiebits approved these changes Apr 10, 2023

View reviewed changes

sebmarkbage added 3 commits April 10, 2023 18:16

Add flag

594b4b0

Diff styles instead of always writing the full set

d629f8b

Also fixes a bug where styles weren't cleared on custom elements.

sebmarkbage force-pushed the diffincommit branch from f3feb8c to 0bd9a67 Compare April 10, 2023 22:30

Add warning if is is set and tries to update

d74688c

sebmarkbage force-pushed the diffincommit branch from 0bd9a67 to d74688c Compare April 10, 2023 23:01

sebmarkbage merged commit ca41adb into facebook:main Apr 10, 2023

kassens added a commit to kassens/react that referenced this pull request Apr 17, 2023

Revert "Diff properties in the commit phase instead of generating an …

87fb8ea

…update payload (facebook#26583)" This reverts commit ca41adb.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diff properties in the commit phase instead of generating an update payload #26583

Diff properties in the commit phase instead of generating an update payload #26583

sebmarkbage commented Apr 10, 2023 •

edited

Loading

sebmarkbage Apr 10, 2023

react-sizebot commented Apr 10, 2023 •

edited

Loading

sebmarkbage Apr 10, 2023

sebmarkbage commented Apr 10, 2023 •

edited

Loading

sophiebits left a comment

sophiebits Apr 10, 2023

sebmarkbage Apr 10, 2023

sophiebits Apr 10, 2023

sebmarkbage Apr 11, 2023

sophiebits Apr 10, 2023

sebmarkbage Apr 10, 2023

sophiebits Apr 10, 2023

sebmarkbage Apr 10, 2023

sebmarkbage Apr 10, 2023

sebmarkbage Apr 10, 2023

sophiebits Apr 10, 2023

sebmarkbage commented Apr 10, 2023

Diff properties in the commit phase instead of generating an update payload #26583

Diff properties in the commit phase instead of generating an update payload #26583

Conversation

sebmarkbage commented Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

react-sizebot commented Apr 10, 2023 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

sebmarkbage commented Apr 10, 2023 • edited Loading

sophiebits 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

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 commented Apr 10, 2023

sebmarkbage commented Apr 10, 2023 •

edited

Loading

react-sizebot commented Apr 10, 2023 •

edited

Loading

sebmarkbage commented Apr 10, 2023 •

edited

Loading