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

Add early return to diffProperties #28842

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

dmytrorykun
Copy link
Contributor

@dmytrorykun dmytrorykun commented Apr 15, 2024

Summary

This PR adds early return to the diff function. We don't need to go through all the entries of nextProps, process and deep-diff the values if nextProps is the same object as prevProps. Roughly 6% of all diffProperties calls can be skipped.

How did you test this change?

RNTester.

@react-sizebot
Copy link

react-sizebot commented Apr 15, 2024

Comparing: b5e5ce8...646fd87

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 = 168.91 kB 168.91 kB = 52.94 kB 52.94 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 170.57 kB 170.57 kB = 53.44 kB 53.44 kB
facebook-www/ReactDOM-prod.classic.js = 590.83 kB 590.83 kB = 103.91 kB 103.91 kB
facebook-www/ReactDOM-prod.modern.js = 566.65 kB 566.65 kB = 100.10 kB 100.10 kB
test_utils/ReactAllWarnings.js Deleted 64.44 kB 0.00 kB Deleted 16.10 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
test_utils/ReactAllWarnings.js Deleted 64.44 kB 0.00 kB Deleted 16.10 kB 0.00 kB

Generated by 🚫 dangerJS against 646fd87

@javache
Copy link
Member

javache commented Apr 18, 2024

Roughly 6% of all diffProperties calls can be skipped.

Can you check how much this is when passChildrenWhenCloningPersistedNodes is enabled?

Copy link
Member

@kassens kassens left a comment

Choose a reason for hiding this comment

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

Seems worthwhile even if it's lower after other optimizations. Do we need a feature flag?

Just make sure to not have the flag too long lived to avoid the extra check in the hot codepath.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

I'm not sure this is safe, we already have an early return for each prop here, after some other logic that seems important:

if (prevProp === nextProp) {
continue; // nothing changed
}

Can we do a PR sync to test this before merging?

@dmytrorykun
Copy link
Contributor Author

@javache I could'n test this change with passChildrenWhenCloningPersistedNodes = true. If I simply set it to true in ReactFabric-dev, the iOS app fails on multiple assertions.

@dmytrorykun
Copy link
Contributor Author

@rickhanlonii I've patched the implementation/ReactFabric* files, and done a good amount of testing locally. Should I do something in addition to that? What do you mean by a "PR sync"?

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Actually since it's feature flagged we can land it and enable the flag for testing

@dmytrorykun dmytrorykun merged commit 0061ca6 into facebook:main Apr 18, 2024
38 checks passed
@dmytrorykun dmytrorykun deleted the diffProperties-early-return branch April 18, 2024 16:24
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
## Summary

This PR adds early return to the `diff` function. We don't need to go
through all the entries of `nextProps`, process and deep-diff the values
if `nextProps` is the same object as `prevProps`. Roughly 6% of all
`diffProperties` calls can be skipped.

## How did you test this change?

RNTester.

DiffTrain build for commit 0061ca6.
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.

6 participants