Repository navigation
Add prototype signals+path-tracking implementation - #2318
markerikson wants to merge 95 commits into
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Size Change: +195 kB (+525.82%) 🆘 Total Size: 232 kB 📦 View Changed
ℹ️ View Unchanged
|
49f63bc to
dda99e1
Compare
|
Saw you working on this from your post on bluesky and got interested in how it works so had a look. I think I found a limitation to this approach that I think is fundamentally unavoidable. Maybe you've figured it out already and intend to document it as a gotcha. If the logic in the selector relies on object identity then the proxies can't really catch that dependency as proxies can't see Object.is or === etc. You can't assume a object is used just because it is read either as that would make everything depend on the root object that changes all the time defeating the entire optimization. https://codesandbox.io/p/sandbox/festive-meadow-q7rk68?file=%2Fsrc%2FApp.tsx%3A7%2C15 |
|
Nice. Avoiding O(n) subscription is great, because proxy-memoize approach is still O(n). |
|
@markerikson import { configureStore } from "@reduxjs/toolkit";
import {
SignalProvider,
useSignalSelector,
useSelector,
useDispatch,
} from "react-redux";
const a = { id: "a" };
const b = { id: "b" };
const initialState: State = {
items: [a, b],
current: a,
};
function reducer(state = initialState, action) {
switch (action.type) {
case "selectA":
return { ...state, current: state.items[0] };
case "selectB":
return { ...state, current: state.items[1] };
default:
return state;
}
}
const store = configureStore({ reducer });
function selectCurrentByReference(state) {
return state.items.find((item) => item === state.current);
}
export default function App() {
return (
<SignalProvider store={store}>
<AppInner />
</SignalProvider>
);
}
function AppInner() {
const dispatch = useDispatch();
return (
<main>
<button onClick={() => dispatch({ type: "selectA" })}>Select A</button>
<button onClick={() => dispatch({ type: "selectB" })}>Select B</button>
<SignalResult />
<NormalResult />
</main>
);
}
function SignalResult() {
const current = useSignalSelector(selectCurrentByReference);
return <p>signal active: {current?.id ?? "none"}</p>;
}
function NormalResult() {
const current = useSelector(selectCurrentByReference);
return <p>normal active: {current?.id ?? "none"}</p>;
} |
65ed737 to
5f96e70
Compare
|
@GabbeV okay, did some research and work around object identity and edge cases. I've added an
Generally, |
|
@markerikson sounds like there are more trickiness there than I even realized. It for sure is unidiomatic redux to have logic around object identity at all so I don't think this is a deal breaker. It is just an example showing that non deterministic logic isn't the only thing preventing this from being a perfect drop in replacement without at least some disclaimers. I'm not sure how you would get away with not wrapping each accessed element in an array find without missing a bunch of real dependencies in a case like The issue I was getting at isn't even that === is always false, it is that === won't register a dependency. I think unwrap is kind of a nice solution to both of those issues though. However I think it needs to not only give access to the wrapped value but also register a dependency on the object changing. Imagine a selector like this |
|
@GabbeV yeah, thanks for bringing up these edge cases! I think I already had the As for the array methods: yeah, I had added the array method wrappers that delegate to the underlying array, specifically to avoid the overhead of creating proxies every time you iterate through an array even if you're just reading. Did some more thinking on this, and I just updated the PR to auto-track the array itself any time you call one of the methods. The key brainstorm I had here is that this whole PR isn't the entire state change detection method - it's a set of heuristics to figure out if it's safe to skip the callbacks and selectors we are pretty sure would be irrelevant because their dependencies haven't changed. So, we need to make sure we never miss an update, but it's okay if we still run some selectors that report "no change, don't re-render". That means that always marking an array as being accessed and needing consideration ought to handle cases like |
18e22cc to
97379d5
Compare
|
@markerikson allowing false positive executions sounds like a smart approach for the arrays if wrapping each item is prohibitive performance wise. I'm wondering if maybe you could reuse proxies as an alternative solution if the performance issue is because of allocations? Not for different objects but for the same object between different selectors. Basically you save the proxy per object in a weakmap or on a hidden symbol property on the object it self. Maybe I am missing something but I believe the issue I'm describing with Here is a more complex example: useSignalSelector(s => {
const a = s.a
const aFooBar = a.foo.bar
const b = s.b
const bFooBar = b.foo.bar
if (a === b) return "same"
if (aFooBar === bFooBar) return "similar"
return "different"
})The proxies can see that you are traversing down s.a.foo.bar and s.b.foo.bar but they can't possibly see that you are checking the identity between s.a and s.b as there is no proxy trap for equality. Imagine the object starting of as "same" but then some property unrelated to foo.bar changes on one of them so that they are no longer the "same" but still "similar".
|
|
@GabbeV I'm inclined to say this is a limitation. Obviously we want selector code to work out of the box. But especially with proxies, we can't cover literally every bit of JS syntax. As it is, we document the "zombie child" case for |
|
Would this make supporting "async react"/concurrency easier or harder? |
|
@nstadigs I'm not sure how it fits architecturally at this point. One reason I'd held off trying to push this idea forward was that the I'm not planning to ship this soon (would need a lot more benchmarking and real-world testing first), but big-picture I wouldn't want to hold off on this "just in case" the React team somehow decides to actually solve that use case. The combination of Meta layoffs, team shuffling, lack of response to the |
|
|
|
FWIW the last commits I pushed that relate to tracking arrays appear to have lost most of the perf gains :( Debating options around correctness vs perf, but also running around and juggling time to work on this. As things stand atm: I'd absolutely appreciate feedback on the correctness of the current implementation in real apps, as well as any more edge cases like the ones discussed above where it turns out this breaks or doesn't propagate updates as expected. As of the current commit I expect it's probably not much faster than |
commit: |
|
Just pushed a sizeable update that adds a lot more diff tracking logic. This appears to restore the overall perf improvements I was seeing earlier. I'll try to post some actual benchmarks later, as well as some analysis on what this does to bundle size. The logic here is complex, but feels plausible given the problem space. Next step is to try to get some more real-world-ish usage scenarios put together. |
|
If the API design is still open to discussion I'd like to propose removing the need to wrap the render tree in a In my mind the Provider is only useful to limit propagation of state updates to sub-trees of the whole React tree, otherwise it is just cumbersome. I'd prefer to opt-in to using a Provider if I needed better performance of scoped updates. Additionally, signal selectors are probably not affected by the use of a higher-level Provider, or need not be.
EDIT: I made a hacky proof of concept work and got all tests to pass here - mikeduminy@3141ffa |
|
@mikeduminy I can take a look at that, but one of my explicit goals for this effort is to make |
|
we also want you to use context for your store because it makes testing significantly simpler |
|
@markerikson in the approach I've followed there the If we had Provider mechanics and people could also use selectors without Providers then isn't it a win-win? @EskiMojo14 I'm not sure I follow, why wouldn't this still be easy to test? You can still wrap your tests with a store creator and use the bound selector from that. |
|
how would the component get the bound hook? |
|
Ah I see. Sure you would need to adjust your testing strategy slightly from the current recommendation if you wanted to test the bound hook specifically (beforeEach In my proposal however I'm not saying that the Provider mechanics need to disappear, just that they are not really needed for this new hook (and arguably for normal hooks too). Equally, the API does not needs to look exactly as described -- it represents a preferred possibility but I'd be just as happy to have a different one. For instance: // create selectors bound to a specific store
const store = configureStore()
const useBoundSelector = createSelectorHook(store.toContext())
// use bound hook with or without a provider
const value = useBoundSelector((state) => state.value)I totally buy the intent to keep this simple and as a drop-in replacement. I guess I'm advocating for exposing a |
|
I'm sure there'll be a |
da2e870 to
9210dad
Compare
markerikson
changed the title
|
Re:
Here are a few gotchas I ran into while working on a related prototype in the Descript codebase: First: if a selector calls any kind of cached helper (e.g., a function created by For example: const getBar = createSelector((state) => state.foo, (foo) => {
return expensiveComputationOf(foo);
});
// within component A
// assuming this one mounts first, it will track `state.foo` as a dependency
useSelector(getBar);
// within component B
// if this one mounts second, `getBar` is already cached on `state`
// and will _not_ track `state.foo` as a dep
// so, next time `state.foo` is updated, component B will not re-render
useSelector(getBar);Second: You need to handle conditional logic when tracking dependencies. For example: function getStuff(state) {
return state.toggle ? state.foo : state.bar;
}If this runs initially with Then if But then if Does this prototype account for either of these two cases? Fwiw, internally we've "solved" these (in prototype form) by (1) using a custom cache builder that replaces all reselect/re-reselect usage and can also do dependency tracking, and (2) updating the tracked dependencies on each call to the selector, rather than just one time on creation. |
|
Yeah, the "conditional logic" is the exact kind of thing this will not handle correctly. Reuse of a shared selector... ew. That looks like basically the same problem in a different form :( Don't think I have a test case for that. Thanks for the comments, will have to try these! |
The six args-memoization tests fail in signals mode: the shared root proxy lets Reselect's argsMemoize short-circuit before any state property is read, so the hook records no dependencies.
The root proxy was cached by state identity like every other proxy, so two evaluations against the same state handed Reselect the same argument. Its argsMemoize layer then short-circuited before any input selector ran, no property was read, and the hook recorded no dependencies. Any hook that evaluated a memoized selector a second time against unchanged state (a shared selector, an inline wrapper after the render-phase swap, a (state, arg) lookup) stopped updating. Only the root skips the cache. Child proxies stay cached by target identity, so result functions still memoize on unchanged inputs.
- find/findLast/filter/slice hand back raw elements plus one identity dependency per element instead of tracking proxies - map is now overridden: scans through the recorder (column signals for primitive results), element results come back raw, tracking proxies returned from elsewhere (ids.map(id => entities[id])) are unwrapped with an identity dependency, and callbacks that embed the element in a fresh object fall back to raw elements under the coarse array signal - Reselect result functions fed by entity-adapter selectAll now run on plain arrays instead of paying a proxy trap per element read
|
@srubin Those were indeed excellent examples of problematic behavior, thank you :) Good news is it turns out this branch already actually handled conditional selectors fine, because the deps are updated on each read. However, reusing a memoized selector was an issue for exactly that reason. The second component would hit the memoization as intended, never read from the state, and not set up a dependency. Took some internal reworking, but that should now be solved. This also chained into rethinking how we handle array methods in some cases. That actually meant that the So, net result is that we have better correctness for those cases, tests that cover them, gave up a bit of perf in some cases, but this branch should still be a net perf win overall. Put that out as https://github.com/reduxjs/react-redux/releases/tag/v9.4.0-alpha.1 |
|
I'm curious about the shape of the memoized selector fix. Does it work even if the memoized selector is not the root level selector? For example: const getBar = createSelector((state) => state.foo, (foo) => {
return expensiveComputationOf(foo);
});
// ...
useSignalSelector((state) => {
return state.a ? getBar(state) : undefined;
});
// another component
useSignalSelector((state) => {
return state.b ? getBar(state) : undefined;
});Is the fix specific to reselect or does it work with any sort of caching, including that of arbitrary (i.e., non-root) parts of the state, or results of other selectors, e.g., function makeMemo((fn) => {
const cache = new WeakMap();
return (arg) => {
let result = cache.get(arg);
if (!result) {
result = fn(arg);
cache.set(arg, result);
}
return result;
};
});
const getExpensive = makeMemo(computeExpensive);
const getEvenMoreExpensive = makeMemo((foo) => computeMoreExpensive(getExpensive(foo)));
// ...
useSignalSelector((state) => {
return getExpensive(state.foo);
});
// another component
useSignalSelector((state) => {
return getExpensive(state.foo);
});
// ...
useSignalSelector((state) => {
return getEvenMoreExpensive(state.foo);
});
// another component
useSignalSelector((state) => {
return getEvenMoreExpensive(state.foo);
}); |
|
The fix was semi-specific to Reselect in that I can take a look and see how these other examples play out. |
This PR:
SignalProvideranduseSignalSelector, designed to optimize the performance of subscriber notifications and selectors in larger apps (at the expense of larger bundle size)Background
For the last several years I have been convinced that there is some way I can use signals and proxies to improve the subscriber+notification performance in large React-Redux apps.
Redux is a simple event emitter, with O(n) subscriber behavior. Every dispatched action triggers a loop over all subscriber callbacks, which in turn normally call
getState()and a selector to determine if thatuseSelectororconnectneeds to re-render. React-Redux moves some of the tracking to be internal to itself and optimizes some subscription aspects, but it's fundamentally still calling O(n) callbacks every time.As with React, this works fine for most apps, but breaks down at large scales. I've talked with teams that had 20,000 connected components, and at that scale just running the callbacks themselves is a major perf issue.
Proxies allow tracking access to nested fields. Signals allow automatically deriving dependencies. More magic, but signals libs automatically only update the dependencies that actually rely on a given updated value.
We can't and won't change the semantics of Redux in terms of immutable updates, subscribing to the store, etc. But, we can try to leverage some of these techniques inside of React-Redux, invisibly to the end user.
Previous Efforts
I've previously tried a couple different variations on this concept:
Neither got very far. They were mostly 1-day efforts that I never got back to. I think the autotracking PR sorta ran but already showed broken cases around field accesses.
Implementation Approach
This attempt pulls together a few different concepts from other libraries, as well as some seemingly new and unique approaches.
It uses a mixture of key path tracking (
"state.nested.field") and signals.When a
useSignalSelectorhook mounts, it runs the actual selector (which as always could be a plain function, Reselect, or some other selector lib). That accesses fields in the state as usual.However, the "state" the selector reads has been wrapped in a proxy. We track the accessed fields, and the terminal reads get turned into a signal for that path. The reads then set up standard signal dependencies, so that we effectively know which state paths this component depends on.
We have a root proxy and previous state value in
SignalProvider. After an action is dispatched, we then do a diff between the old and new state, and calculate which state paths got changed. For those changed values, we callsignal.set(newValue)internally. The updated signals then trigger theeffect()s inside ofuseSignalSelector, which in turn trigger notifications of React.There's a bunch of other internal implementation details I can write up later (like optimizing array updates via an entity-adapter-like ID tracking table, and reusing the same array method overrides approach I used on Immer), but that's the core idea.
Overall, the tradeoffs are:
useSelector. I haven't actually measured how big yet :) I'd assume on the order of 5-10K minified.Usage
There's two new exports,
SignalProvideranduseSignalSelector. They ought to both be drop-in replacements forProvideranduseSelector. Swap inSignalProvider, which passes down the existing context values + the signal data. Then calluseSignalSelectorin your components, same semantics, no user-visible difference.Status and Notes
Status
Alpha-level, but it appears to be stable, is passing the new test suite so far, and is a significant improvement in my benchmarks suite (see below).
Haven't tried this in a real app yet, but the benchmarks setup is a suite of small example apps. So, between that and the unit tests, I'm pretty confident this runs in the cases I've tried so far.
Development Process
Getting this out up-front. The implementation here is 100% AI-written.
To be clear, this is engineered, not vibe-coded. I used my own standard "human-in-the-loop" AI engineering workflow. My ideas, fleshed out and implemented, over the course of a couple weeks. I reviewed all the lib code before each commit. (Maybe not all the tests :) )
I was also able to use AI to heavily revamp my longstanding React-Redux benchmarks repo: updating the build system, reviewing the existing scenarios, deleting dead ones, implementing better scenarios that provided better coverage of different use cases, and completely reworking what metrics I'm capturing so they're a lot more meaningful.
This is a POC, but it's code that I very intentionally drove the development of. I feel very confident that the current implementation actually runs as expected and implements the approaches I've worked through. I wouldn't even PR this unless I felt it was sufficiently solid enough to be public and tried out in real apps.
Performance
So, is this actually any better? :) (you would hope so... otherwise why would I even have taken the time to file this PR and write up this explanation?)
Based on my heavily revised and updated React-Redux benchmarks suite:
YES! This branch appears to be 20-40% faster in total scripting time per scenario than the current 9.x release
Here's a summary:
And the per-scenario details from my benchmarks suite:
Performance benchmark numbers
So, overall, it's meaningfully faster in 12 of the 14 benchmark scenarios, one is even, and the only case where it loses is an unrealistic "one component subscribed to hundreds of slices" scenario.
Next Steps?
At this point I think this is ready to be tried out in real-world apps. I'm sure there's edge cases that we aren't handling, just because real code is complex and we're doing tricky work here :) I'd love to know where this breaks, and if it appears to actually improve perf.
I need to do some testing to see how much this increases bundle size, as well as reviewing the overall test cases here and seeing what else we ought to cover.
For the record I do not plan on actually replacing the existing
ProvideranduseSelectorimplementations. This is an alternative, not a replacement, primarily aimed at apps with large codebases and many subscriptions. I might ship an alternate entry point that would swap to the signal versions, and if you want to use them everywhere you could alias something like"react-redux/signals"as the import in your bundler setup.CI Builds
As usual, see the CodeSandbox CI link below for the latest commit preview build.
Current one is: