-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Add early return to diffProperties #28842
Conversation
Comparing: b5e5ce8...646fd87 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
88b3048
to
ef7101d
Compare
ef7101d
to
646fd87
Compare
Can you check how much this is when |
There was a problem hiding this 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.
There was a problem hiding this 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:
react/packages/react-native-renderer/src/ReactNativeAttributePayload.js
Lines 336 to 338 in afea1d0
if (prevProp === nextProp) { | |
continue; // nothing changed | |
} |
Can we do a PR sync to test this before merging?
@javache I could'n test this change with |
@rickhanlonii I've patched the |
There was a problem hiding this 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
## 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.
Summary
This PR adds early return to the
diff
function. We don't need to go through all the entries ofnextProps
, process and deep-diff the values ifnextProps
is the same object asprevProps
. Roughly 6% of alldiffProperties
calls can be skipped.How did you test this change?
RNTester.