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

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 2 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 {ContextDependencyWithCompare} 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>
| ContextDependencyWithCompare<mixed, mixed> = null;

function nextHook(): null | Hook {
const hook = currentHook;
Expand Down
89 changes: 88 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,13 @@ function updateWorkInProgressHook(): Hook {
return workInProgressHook;
}

function unstable_useContextWithBailout<T>(
context: ReactContext<T>,
compare: (T => mixed) | null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

): T {
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 +3701,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 +3744,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 +3787,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 +3830,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 +4047,14 @@ if (__DEV__) {
return mountOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnMountInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(context: ReactContext<T>, compare: (T => mixed) | null): T {
currentHookNameInDev = 'useContext';
mountHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}

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

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

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

InvalidNestedHooksDispatcherOnMountInDEV = {
readContext<T>(context: ReactContext<T>): T {
Expand Down Expand Up @@ -4766,6 +4826,15 @@ if (__DEV__) {
return mountOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(context: ReactContext<T>, compare: (T => 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 +5041,15 @@ if (__DEV__) {
return updateOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(context: ReactContext<T>, compare: (T => 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 +5256,13 @@ if (__DEV__) {
return rerenderOptimistic(passthrough, reducer);
};
}
if (enableContextProfiling) {
(InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).unstable_useContextWithBailout =
function <T>(context: ReactContext<T>, compare: (T => mixed) | null): T {
currentHookNameInDev = 'useContext';
warnInvalidHookAccess();
updateHookTypesDev();
return unstable_useContextWithBailout(context, compare);
};
}
}
133 changes: 126 additions & 7 deletions packages/react-reconciler/src/ReactFiberNewContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
Fiber,
ContextDependency,
Dependencies,
ContextDependencyWithCompare,
} from './ReactInternalTypes';
import type {StackCursor} from './ReactFiberStack';
import type {Lanes} from './ReactFiberLane';
Expand Down Expand Up @@ -51,6 +52,8 @@ import {
getHostTransitionProvider,
HostTransitionContext,
} from './ReactFiberHostContext';
import isArray from '../../shared/isArray';
import {enableContextProfiling} from '../../shared/ReactFeatureFlags';

const valueCursor: StackCursor<mixed> = createCursor(null);

Expand All @@ -70,7 +73,10 @@ if (__DEV__) {
}

let currentlyRenderingFiber: Fiber | null = null;
let lastContextDependency: ContextDependency<mixed> | null = null;
let lastContextDependency:
| ContextDependency<mixed>
| ContextDependencyWithCompare<mixed, mixed>
| null = null;
let lastFullyObservedContext: ReactContext<any> | null = null;

let isDisallowedContextReadInDEV: boolean = false;
Expand Down Expand Up @@ -400,8 +406,24 @@ function propagateContextChanges<T>(
findContext: for (let i = 0; i < contexts.length; i++) {
const context: ReactContext<T> = contexts[i];
// Check if the context matches.
// TODO: Compare selected values to bail out early.
if (dependency.context === context) {
if (enableContextProfiling) {
const compare = dependency.compare;
if (compare != null) {
const newValue = isPrimaryRenderer
? dependency.context._currentValue
: dependency.context._currentValue2;
if (
!checkIfComparedContextValuesChanged(
dependency.lastComparedValue,
compare(newValue),
)
) {
// Compared value hasn't changed. Bail out early.
continue findContext;
}
}
}
// Match! Schedule an update on this fiber.

// In the lazy implementation, don't mark a dirty flag on the
Expand Down Expand Up @@ -641,6 +663,28 @@ function propagateParentContextChanges(
workInProgress.flags |= DidPropagateContext;
}

function checkIfComparedContextValuesChanged(
oldComparedValue: mixed,
newComparedValue: mixed,
): boolean {
if (isArray(oldComparedValue) && isArray(newComparedValue)) {
jackpope marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

for (
let i = 0;
i < oldComparedValue.length && i < newComparedValue.length;
i++
) {
if (!is(newComparedValue[i], oldComparedValue[i])) {
return true;
}
}
} else {
jackpope marked this conversation as resolved.
Show resolved Hide resolved
if (!is(newComparedValue, oldComparedValue)) {
return true;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return false;
}

export function checkIfContextChanged(
currentDependencies: Dependencies,
): boolean {
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if (
checkIfComparedContextValuesChanged(
dependency.lastComparedValue,
dependency.compare(newValue),
)
) {
return true;
}
} else {
if (!is(newValue, oldValue)) {
return true;
}
}
dependency = dependency.next;
}
Expand Down Expand Up @@ -694,6 +749,21 @@ export function prepareToReadContext(
}
}

export function readContextAndCompare<C>(
context: ReactContext<C>,
compare: (C => mixed) | null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can make this arg required if you make the suggested change

): C {
if (!enableLazyContextPropagation) {
return readContext(context);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to error if both enableLazyContextPropagation and enableContextProfiling don't pass


return readContextForConsumer_withCompare(
currentlyRenderingFiber,
context,
compare,
);
}

export function readContext<T>(context: ReactContext<T>): T {
if (__DEV__) {
// This warning would fire if you read context inside a Hook like useMemo.
Expand Down Expand Up @@ -721,10 +791,59 @@ export function readContextDuringReconciliation<T>(
return readContextForConsumer(consumer, context);
}

function readContextForConsumer<T>(
type ContextCompare<C, V> = C => V | null;

function readContextForConsumer_withCompare<C, S>(
consumer: Fiber | null,
context: ReactContext<T>,
): T {
context: ReactContext<C>,
compare: (C => S) | null,
): C {
const value = isPrimaryRenderer
? context._currentValue
: context._currentValue2;

if (lastFullyObservedContext === context) {
// Nothing to do. We already observe everything in this context.
} else {
const contextItem = {
context: ((context: any): ReactContext<mixed>),
memoizedValue: value,
next: null,
compare: compare ? ((compare: any): ContextCompare<mixed, mixed>) : null,
lastComparedValue: compare != null ? compare(value) : null,
};

if (lastContextDependency === null) {
if (consumer === null) {
throw new Error(
'Context can only be read while React is rendering. ' +
'In classes, you can read it in the render method or getDerivedStateFromProps. ' +
'In function components, you can read it directly in the function body, but not ' +
'inside Hooks like useReducer() or useMemo().',
);
}

// This is the first dependency for this component. Create a new list.
lastContextDependency = contextItem;
consumer.dependencies = {
lanes: NoLanes,
firstContext: contextItem,
};
if (enableLazyContextPropagation) {
consumer.flags |= NeedsPropagation;
}
} else {
// Append a new context item.
lastContextDependency = lastContextDependency.next = contextItem;
}
}
return value;
}

function readContextForConsumer<C>(
consumer: Fiber | null,
context: ReactContext<C>,
): C {
const value = isPrimaryRenderer
? context._currentValue
: context._currentValue2;
Expand Down
Loading
Loading