Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Fabric-specific version of ReactNativeAttributesPayload #28841

Conversation

dmytrorykun
Copy link
Contributor

@dmytrorykun dmytrorykun commented Apr 15, 2024

Summary

This PR introduces Fabric-only version of ReactNativeAttributesPayload. It is a copy-paste of ReactNativeAttributesPayload.js, and is called ReactNativeAttributesPayloadFabric.js.
The idea behind this change is that certain optimizations in prop diffing may actually be a regression on the old architecture. For example, removing custom diffing may result in larger updateProps payloads. Which is, I guess, fine with JSI, but might be a problem with the bridge.

How did you test this change?

There should be no runtime effect of this change.

@react-sizebot
Copy link

react-sizebot commented Apr 15, 2024

Comparing: b498834...ad43eec

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.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 494.06 kB 494.06 kB = 88.22 kB 88.21 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 498.86 kB 498.86 kB = 88.93 kB 88.92 kB
facebook-www/ReactDOM-prod.classic.js = 591.22 kB 591.22 kB = 103.96 kB 103.96 kB
facebook-www/ReactDOM-prod.modern.js = 567.44 kB 567.44 kB = 100.36 kB 100.36 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/implementations/ReactNativeRenderer-profiling.fb.js = 400.74 kB 399.47 kB = 69.73 kB 69.57 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js = 373.51 kB 372.23 kB = 65.42 kB 65.28 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against ad43eec

@dmytrorykun dmytrorykun force-pushed the create-ReactNativeAttributesPayloadFabric branch 2 times, most recently from cd7015c to 2012c43 Compare April 23, 2024 15:04
@dmytrorykun dmytrorykun marked this pull request as ready for review April 23, 2024 15:04
@dmytrorykun dmytrorykun force-pushed the create-ReactNativeAttributesPayloadFabric branch from 2012c43 to 573c608 Compare May 7, 2024 09:51
} else {
return diffProperties(updatePayload, emptyObject, props, validAttributes);
}
// TODO: Fast path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this TODO if we're only going to do so in ReactNativeAttributePayloadFabric. I don't believe we should be making any other changes to Paper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just how the code looked before I added fastAddProperties.

@dmytrorykun dmytrorykun merged commit 7039834 into facebook:main May 7, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request May 7, 2024
## Summary

This PR introduces Fabric-only version of
`ReactNativeAttributesPayload`. It is a copy-paste of
`ReactNativeAttributesPayload.js`, and is called
`ReactNativeAttributesPayloadFabric.js`.
The idea behind this change is that certain optimizations in prop
diffing may actually be a regression on the old architecture. For
example, removing custom diffing may result in larger updateProps
payloads. Which is, I guess, fine with JSI, but might be a problem with
the bridge.

## How did you test this change?

There should be no runtime effect of this change.

DiffTrain build for commit 7039834.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants