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.

Sign up for GitHub

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

Jump to bottom

Add unstable context bailout for profiling #30407

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Add unstable context bailout for profiling #30407

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c6a3e4d
Add unstable context access API for internal profiling
Jul 10, 2024
b0f5692
Fork readContextForConsumer
Jul 23, 2024
f106363
Check length of compared arrays
Jul 24, 2024
0f64b02
Require array return
Jul 26, 2024
d278b8f
Rename compare->select
Jul 26, 2024
96136ab
f
Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/react-debug-tools/src/ReactDebugHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
REACT_CONTEXT_TYPE,
} from 'shared/ReactSymbols';
import hasOwnProperty from 'shared/hasOwnProperty';
import type {ContextDependencyWithSelect} from '../../react-reconciler/src/ReactInternalTypes';

type CurrentDispatcherRef = typeof ReactSharedInternals;

Expand Down Expand Up @@ -155,7 +156,10 @@ function getPrimitiveStackCache(): Map<string, Array<any>> {

let currentFiber: null | Fiber = null;
let currentHook: null | Hook = null;
let currentContextDependency: null | ContextDependency<mixed> = null;
let currentContextDependency:
| null
| ContextDependency<mixed>
| ContextDependencyWithSelect<mixed> = null;

function nextHook(): null | Hook {
const hook = currentHook;
Expand Down
113 changes: 112 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
enableUseDeferredValueInitialArg,
disableLegacyMode,
enableNoCloningMemoCache,
enableContextProfiling,
} from 'shared/ReactFeatureFlags';
import {
REACT_CONTEXT_TYPE,
Expand Down Expand Up @@ -81,7 +82,11 @@ import {
ContinuousEventPriority,
higherEventPriority,
} from './ReactEventPriorities';
import {readContext, checkIfContextChanged} from './ReactFiberNewContext';
import {
readContext,
readContextAndCompare,
checkIfContextChanged,
} from './ReactFiberNewContext';
import {HostRoot, CacheComponent, HostComponent} from './ReactWorkTags';
import {
LayoutStatic as LayoutStaticEffect,
Expand Down Expand Up @@ -1053,6 +1058,16 @@ function updateWorkInProgressHook(): Hook {
return workInProgressHook;
}

function unstable_useContextWithBailout<T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
if (compare === null) {
return readContext(context);
}
return readContextAndCompare(context, compare);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense

}

// NOTE: defining two versions of this function to avoid size impact when this feature is disabled.
// Previously this function was inlined, the additional `memoCache` property makes it not inlined.
let createFunctionComponentUpdateQueue: () => FunctionComponentUpdateQueue;
Expand Down Expand Up @@ -3689,6 +3704,10 @@ if (enableAsyncActions) {
if (enableAsyncActions) {
(ContextOnlyDispatcher: Dispatcher).useOptimistic = throwInvalidHookError;
}
if (enableContextProfiling) {
(ContextOnlyDispatcher: Dispatcher).unstable_useContextWithBailout =
throwInvalidHookError;
}

const HooksDispatcherOnMount: Dispatcher = {
readContext,
Expand Down Expand Up @@ -3728,6 +3747,10 @@ if (enableAsyncActions) {
if (enableAsyncActions) {
(HooksDispatcherOnMount: Dispatcher).useOptimistic = mountOptimistic;
}
if (enableContextProfiling) {
(HooksDispatcherOnMount: Dispatcher).unstable_useContextWithBailout =
unstable_useContextWithBailout;
}

const HooksDispatcherOnUpdate: Dispatcher = {
readContext,
Expand Down Expand Up @@ -3767,6 +3790,10 @@ if (enableAsyncActions) {
if (enableAsyncActions) {
(HooksDispatcherOnUpdate: Dispatcher).useOptimistic = updateOptimistic;
}
if (enableContextProfiling) {
(HooksDispatcherOnUpdate: Dispatcher).unstable_useContextWithBailout =
unstable_useContextWithBailout;
}

const HooksDispatcherOnRerender: Dispatcher = {
readContext,
Expand Down Expand Up @@ -3806,6 +3833,10 @@ if (enableAsyncActions) {
if (enableAsyncActions) {
(HooksDispatcherOnRerender: Dispatcher).useOptimistic = rerenderOptimistic;
}
if (enableContextProfiling) {
(HooksDispatcherOnRerender: Dispatcher).unstable_useContextWithBailout =
unstable_useContextWithBailout;
}

let HooksDispatcherOnMountInDEV: Dispatcher | null = null;
let HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher | null = null;
Expand Down Expand Up @@ -4019,6 +4050,17 @@ if (__DEV__) {
return mountOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnMountInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
mountHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

HooksDispatcherOnMountWithHookTypesInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4200,6 +4242,17 @@ if (__DEV__) {
return mountOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

HooksDispatcherOnUpdateInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4380,6 +4433,17 @@ if (__DEV__) {
return updateOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

HooksDispatcherOnRerenderInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4560,6 +4624,17 @@ if (__DEV__) {
return rerenderOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

InvalidNestedHooksDispatcherOnMountInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4766,6 +4841,18 @@ if (__DEV__) {
return mountOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
warnInvalidHookAccess();
mountHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

InvalidNestedHooksDispatcherOnUpdateInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4972,6 +5059,18 @@ if (__DEV__) {
return updateOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
warnInvalidHookAccess();
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

InvalidNestedHooksDispatcherOnRerenderInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -5178,4 +5277,16 @@ if (__DEV__) {
return rerenderOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(
context: ReactContext<T>,
compare: (T => Array<mixed>) | null,
): T {
currentHookNameInDev = 'useContext';
warnInvalidHookAccess();
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}
}
Loading
Loading