[internal] Performance: fast useStore implementation#3445
[internal] Performance: fast useStore implementation#3445michaldudak merged 20 commits intomui:masterfrom
useStore implementation#3445Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a performance optimization for the useStore hook by implementing a "fast hooks" pattern that reduces store subscriptions from multiple-per-component to one-per-component. The optimization wraps components to establish an instance context, enabling useSyncExternalStore to be called once while accumulating multiple selectors. This is applied to React 19+ and results in 15-35% faster mount times for tooltip triggers.
Key changes:
- New
fastHooksmodule with component wrappers (create,createRef) that manage per-instance hook state useStoreFastimplementation that batches multipleuseStorecalls into a single subscription- Updated
TooltipRootandTooltipTriggerto use the new fast hooks wrappers
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
packages/utils/src/fastHooks.ts |
New module providing component wrappers that establish instance context for optimized hook batching |
packages/utils/src/store/useStore.ts |
Adds useStoreFast implementation that leverages instance context to batch store subscriptions |
packages/react/src/tooltip/trigger/TooltipTrigger.tsx |
Wraps component with fastHooks.createRef to enable store subscription optimization |
packages/react/src/tooltip/root/TooltipRoot.tsx |
Wraps component with fastHooks.create to enable store subscription optimization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| after: (instance: any) => void; | ||
| }; | ||
|
|
||
| const hooks: HookType[] = []; |
There was a problem hiding this comment.
Singleton in the utils package here, but I guess it's not harmful, even if utils package gets duplicated, right?
There was a problem hiding this comment.
I've been considering de-duplicating the global state at runtime (const hooks = globalThis.fastHooks_v1 ??= []), but it's indeed not harmful. There is no logical bug from doing so, only lesser gains.
There was a problem hiding this comment.
Actually there wouldn't be less performance gains, there would only be more bundle size. Even if another library were to take BUI components and add wrappers around it with a different fast hooks version, those wrappers would still be different components that would use their own version of fast hooks, without interfering with this one. Using a globalThis state might actually introduce more bugs and break stuff, and using a module-global variable like here is more sound.
There was a problem hiding this comment.
Did you consider a more explicit approach, where a single store.useState accepts multiple selectors and composes them together?
There was a problem hiding this comment.
Not really possible, some store calls might be spread across multiple files & hooks. The "popup" logic is shared by many components (tooltip, menu, popover) with hooks like useFocus or useTriggerRegistration accessing the common state interface from outside the component.
There was a problem hiding this comment.
Gotcha, so in this example of MenuRoot we want to batch all store.useState calls, while some of them might be made in custom hooks, correct?
There was a problem hiding this comment.
Yes, that's correct.
|
|
||
| let result; | ||
| try { | ||
| currentInstance = instance; |
There was a problem hiding this comment.
WDYT about attaching the current instance to the component function instead of keeping it global to solve the concurrent rendering problem?
There was a problem hiding this comment.
What do you mean by attaching to the component function? And which concurrent rendering problem?
There was a problem hiding this comment.
which concurrent rendering problem
To expand on that, AFAIU concurrent rendering can interrupt rendering a React commit, but it does so in between component renders — it doesn't stack component render functions on top of another. In other words, concurrent renderering is not parallel rendering. So as long as one component render function exits/enters without another component render function being entered, then this implementation is sound.
Even if React was entering multiple component functions, we could use this implementation to push/pop the current instance on the stack, but I think it's safe to assume that React doesn't and won't ever do so — it would complexify their codebase substantially.
const previousInstance = currentInstance
currentInstance = instance
const result = render(...args)
currentInstance = previousInstanceSo my analysis is that concurrent rendering is not an issue, despite what copilot commented above. Server rendering is no different as we're always in a single-threaded context in javascript.
There was a problem hiding this comment.
What do you mean by attaching to the component function?
I meant having the instance and hooks as properties on the component function. But this won't help if React ever interleaves render functions of two instances of the same component function.
My biggest worry is that this breaks the assuptions React has about user code - it makes renders unpure. While all the tests pass today, we can't be sure React won't change anything in their internal implementation that will make it introduce subtle bugs. Also we don't test our implementation with suspended components. Concurrent rendering is poorly documented, so I'm not sure what's exactly going on there - perhaps that's the main reason I'm cautious.
But perhaps someone with better knowledge of React internals could chime in here. @eps1lon, can I pick your brains (or is there someone who can know this better)? Is the solution proposed here safe enough?
There was a problem hiding this comment.
My biggest worry is that this breaks the assuptions React has about user code - it makes renders unpure
React hooks are impure to start with — they're stateful functions that depend on hidden global state, and they even have weird rules that don't exists anywhere else. If React hooks are impure, then render functions that use hooks are also impure, by definition. This is just to say that complete purity has never been an option, no language can ever be completely pure, not even Haskell, but the absence of purity also doesn't mean that we can't make a function idempotent and well-behaved. The trick is just to make our impurity dependent on stuff that comes out of React hooks, which currentInstance does as it's dependent on a ref.
But this won't help if React ever interleaves render functions of two instances of the same component function.
I haven't been able to imagine an issue with that. This hook checks if the current render differs from the previous one. If there is interleaving of different React commits, from this hook's point of view it's the same thing as if React was rendering the commits in full (aka without concurrent rendering): render functions are called one after the other. The only problem I could imagine is if React tries to render an older commit after a newer commit — but that would break a lot of user code (think about the hacks we sometimes put in effects & refs), so I feel that assumption is safe to make.
Also we don't test our implementation with suspended components
I have considered that case. Essentially, suspense throws stuff around, that's why I'm wrapping with try / finally — and finally is guaranteed by the language spec to be executed before the stack exits the function. I also placed the instance.didInitialize = true inside the try block (and not the finally block) precisely because I have considered the scenario where Suspense interrupts a component's first render(s) mid-way.
Signed-off-by: Rom Grk <romgrk@users.noreply.github.com>
|
|
||
| let result; | ||
| try { | ||
| currentInstance = instance; |
There was a problem hiding this comment.
What do you mean by attaching to the component function?
I meant having the instance and hooks as properties on the component function. But this won't help if React ever interleaves render functions of two instances of the same component function.
My biggest worry is that this breaks the assuptions React has about user code - it makes renders unpure. While all the tests pass today, we can't be sure React won't change anything in their internal implementation that will make it introduce subtle bugs. Also we don't test our implementation with suspended components. Concurrent rendering is poorly documented, so I'm not sure what's exactly going on there - perhaps that's the main reason I'm cautious.
But perhaps someone with better knowledge of React internals could chime in here. @eps1lon, can I pick your brains (or is there someone who can know this better)? Is the solution proposed here safe enough?
Co-authored-by: Michał Dudak <michal.dudak@gmail.com> Signed-off-by: Rom Grk <romgrk@users.noreply.github.com>
|
I tested it quite extensively today and couldn't find any issues. Add comments to the code explaining the implementation and rationale to reduce the number of raised eyebrows of external contributors and future us. We're going to have a 1.0.1 release next week, and I'd like to have this merged in after the release, so it lives on master for another month, just in case we find something unexpected. |
|
This is ready for merging on my side, please approve if there's nothing blocking. |
| hooks.push(hook); | ||
| } | ||
|
|
||
| export function fastComponent<P extends object, E extends HTMLElement, R extends React.ReactNode>( |
There was a problem hiding this comment.
Should we add documentation for the next person to explain when to use this and its limits?
Beyond contributor value, there also seems to be user value: in the eye of someone who is used to canonical React code, this is advanced, so it looks like an opportunity to teach them something, so to gain more trust from them in us.
Problem
The
useStore()hook can be called many times per component. The naive implementation defers to React'suseSyncExternalStore(), which creates one store subcription per call, which means one component ends up with multiple subscriptions to the store. This is unefficient because the component re-renders as an unit, and only requires 1 subscription.Solution
This PR introduces a wrapper around components to establish an
instancecontext object per component instance, which we can use to calluseSyncExternalStore()at most once per component. Subsequent calls just accumulate the selectors in an array, without callinguseSyncExternalStore()again.The one downside of this solution is that we need to wrap components to make store accesses faster, e.g.:
Results
This change results in a 15-35% decrease in mount time for the contained tooltip triggers benchmark.
TBD: re-run benchmarks