Add unstable context bailout for profiling #30407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: f7ee804...ce4479c 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
|
jackpope
force-pushed
the
profile-context-access
branch
from
26d3784 to
262e651
Compare
gnoff
reviewed
Jul 22, 2024
jackpope
force-pushed
the
profile-context-access
branch
from
262e651 to
b3885c2
Compare
jackpope
force-pushed
the
profile-context-access
branch
from
b3885c2 to
b0f5692
Compare
gnoff
reviewed
Jul 25, 2024
| context: ReactContext<T>, | ||
| compare: (T => mixed) | null, | ||
| ): T { | ||
| return readContextAndCompare(context, compare); |
There was a problem hiding this comment.
probably should make a null compare function equivalent to just returning the context value. There is logic elsewhere that checks if a compare is passed in but this can lead to maybe confusing code paths if you start with a compare function and then remove it on update or vice versa. Basically if you are using this hook the compare should be required internally and we can make the argument optional for convenience by mapping it to some intuitive default compare function.
That said since this is a compiler target maybe just make the second argument required?
There was a problem hiding this comment.
Actually now that I am thinking about the fact that the compiler will always return an array from the compare function there really is no identity mapping. If the context value was a class instance how are you planning on reprsenting that? Just an array with the instance in the first slot? I guess that works and you can opt to use multiple indexes if you detect that there is some kind of object destructuring going on?
Still might be worth determining what a null compare argument means semantically or just make it required to avoid the issue
There was a problem hiding this comment.
This is where the API is designed around testing over direct usage. The goal is to run an A/B performance test. We want some compiler output like
const {foo, bar} = useContextWithBailoutTest(MyContext, (c) => [c.foo, c.bar])
Then we'll define that hook in the app with experiment check like
function useContextWithBailoutTest(context, compare) {
const inBailoutExperiment = bailoutExperimentCheck();
return unstable_useContextWithBailout(context, inBailoutExperiment ? compare : null)
}
The experiment check will be stable so we wouldn't go between a compare function and null on any update.
We could alternatively pass in some kind of null function
return unstable_useContextWithBailout(context, inBailoutExperiment ? compare : () => {})
The goal of passing null directly was to prevent setting the extra properties on the dependency and running the compare in propagation for the control side of the test. It's likely negligible in real apps but does show up as additional overhead on benchmarks with very fast updates.
| @@ -659,8 +703,19 @@ export function checkIfContextChanged( | |||
| ? context._currentValue | |||
| : context._currentValue2; | |||
| const oldValue = dependency.memoizedValue; | |||
| if (!is(newValue, oldValue)) { | |||
| return true; | |||
| if (enableContextProfiling && dependency.compare != null) { | |||
There was a problem hiding this comment.
This is where my other comment re: expecting the compare to always be present comes in. Seems potentially if you go from no compare to compare and vice versa since you switch to doing is based comparison and ignoring the last compared value
| @@ -694,6 +749,21 @@ export function prepareToReadContext( | |||
| } | |||
| } | |||
|
|
|||
| export function readContextAndCompare<C>( | |||
| context: ReactContext<C>, | |||
| compare: (C => mixed) | null, | |||
There was a problem hiding this comment.
can make this arg required if you make the suggested change
Comment on lines
756
to
758
| if (!enableLazyContextPropagation) { | ||
| return readContext(context); | ||
| } |
There was a problem hiding this comment.
I get why this makes sense logically but seems more appropriate maybe to just make this an error since it's not really valid to build the new flag without the old flag. seems almost certain that this would cause CI to fail if one were to test a build with that particular flag configuration
There was a problem hiding this comment.
Updated to error if both enableLazyContextPropagation and enableContextProfiling don't pass
| @@ -1053,6 +1058,13 @@ function updateWorkInProgressHook(): Hook { | |||
| return workInProgressHook; | |||
| } | |||
|
|
|||
| function unstable_useContextWithBailout<T>( | |||
| context: ReactContext<T>, | |||
| compare: (T => mixed) | null, | |||
There was a problem hiding this comment.
The name isn't super important b/c it's not going to be observed by anyone but I feel like compare is a confusing name for this argument because it doesn't do any comparison it just selects a value. why not call it select or something. I think you can land with compare just realized it was just chafing my automatic mental model while reviewing the PR
| oldComparedValue: mixed, | ||
| newComparedValue: mixed, | ||
| ): boolean { | ||
| if (isArray(oldComparedValue) && isArray(newComparedValue)) { |
There was a problem hiding this comment.
Let's comment why this array check is safe. You mentioned in the comments but would be good to clarify that returning an array an implicit contract and we don't expect to return other things for now
Comment on lines
680
to
684
| } else { | ||
| if (!is(newComparedValue, oldComparedValue)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not really sure it's worth leaving this in b/c you can't really just change the compiler without also changing this comparison logic since you'd end up with incidental index based comparisons if you ever didn't return a wrapping array. Since its de facto part of the API might as well make anything that violates this API error so you know you messed something up in the compiler
There was a problem hiding this comment.
Removed the is check here in favor of leaning into the array return contract. Also typed the inputs as Arrays and check against lastSelectedValue being available before entering comparison function
added 2 commits
July 26, 2024 11:48
gnoff
approved these changes
Jul 26, 2024
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: f01152c777fa2877d4118894557a6b96d572e18f Pull Request resolved: #30548
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 6a8d58e9f18a34c00e7555fe1e961d6f2420b01c Pull Request resolved: #30548
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 2, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 45b59f0cc825b010ec9c034ceea447fafeef6ed6 Pull Request resolved: #30548
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 8a1ba69989c13939c6646b7ac6fc5255b4770ac2 Pull Request resolved: #30548
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 6, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 7, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 7, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]
gsathya
added a commit
that referenced
this pull request
Aug 7, 2024
*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 92d0f6ff2fe95cda93f66786f56e97ba9ace95fa Pull Request resolved: #30548
hoxyq
added a commit
that referenced
this pull request
Aug 30, 2024
…ks and useContextWithBailout (#30837) Related - #30407. This is experimental-only and FB-only hook. Without these changes, inspecting an element that is using this hook will throw an error, because this hook is missing in Dispatcher implementation from React DevTools, which overrides the original one to build the hook tree.  One nice thing from it is that in case of any potential regressions related to this experiment, we can quickly triage which implementation of `useContext` is used by inspecting an element in React DevTools. Ideally, I should've added some component that is using this hook to `react-devtools-shell`, so it can be manually tested, but I can't do it without rewriting the infra for it. This is because this hook is only available from fb-www builds, and not experimental.
jackpope
added a commit
to jackpope/react
that referenced
this pull request
Dec 16, 2024
This reverts commit 1350a85.
jackpope
added a commit
to jackpope/react
that referenced
this pull request
Dec 16, 2024
This reverts commit 1350a85.
jackpope
added a commit
to jackpope/react
that referenced
this pull request
Dec 16, 2024
This reverts commit 1350a85.
jackpope
added a commit
to jackpope/react
that referenced
this pull request
Dec 16, 2024
This reverts commit 1350a85.
jackpope
added a commit
to jackpope/react
that referenced
this pull request
Dec 16, 2024
This reverts commit 1350a85.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This API is not intended to ship. This is a temporary unstable hook for internal performance profiling.
This PR exposes
unstable_useContextWithBailout, which takes a compare function in addition to Context. The comparison function is run to determine if Context propagation and render should bail out earlier.unstable_useContextWithBailoutreturns the full Context value, same asuseContext.We can profile this API against
useContextto better measure the cost of Context value updates and gather more data around propagation and render performance.The bailout logic and test cases are based on #20646
Additionally, this implementation allows multiple values to be compared in one hook by returning a tuple to avoid requiring additional Context consumer hooks in some cases.