Move out internal-state from redux store #633
Conversation
pradeepnschrodinger
requested review from
abhisheksingh2410,
AmanGupta2708,
miskreant,
mycrobe,
novakps,
somavara and
tchoiSchrodinger
| @@ -0,0 +1,102 @@ | |||
| /** | |||
There was a problem hiding this comment.
The component here is used for the "Dynamic Row Heights" example.
It renders a table with 100k rows where each row has a different row height.
Without moving internal-state out of the redux store, this example runs slow in FDT v2.
| @@ -188,7 +188,7 @@ function calculateRenderedRowRange(state, scrollAnchor) { | |||
|
|
|||
| // Handle a case where the offset puts the first row fully offscreen | |||
| // This can happen if availableHeight & maxAvailableHeight are different | |||
| const { storedHeights } = state; | |||
| const { storedHeights } = state.getInternal(); | |||
There was a problem hiding this comment.
storedHeights is no longer a direct part of the redux store state. Instead getInternal is used to access it.
The rest of the code changes in the PR are similar: Accessing internal state like storedHeights, rowBufferSet, and rowOffsetIntervalTree are now done through the getter.
| @@ -32,6 +41,8 @@ import { createSlice } from '@reduxjs/toolkit'; | |||
| * @return {!Object} | |||
| */ | |||
| function getInitialState() { | |||
| const internalState = createInternalState(); | |||
There was a problem hiding this comment.
We create the internal state here and assign it to the local variable internalState.
The getter defined down below -- getInternal -- accesses internal state via closure.
| * TODO (jordan) investigate if we want to move this to local or scoped state | ||
| /** | ||
| * Internal state is only used by reducers. | ||
| * NOTE (jordan, pradeep): Internal state is altered in place, so don't trust it for redux history or immutabability checks. |
There was a problem hiding this comment.
@wcjordan, pinging you for sanity checking since you have the most context regarding "internal state".
There was a problem hiding this comment.
Oh man, it's been too long for me to have much of an opinion here. But no strong concerns from my end...
The only thing I would double check is, my understanding of redux is that it's designed so reducers are pure functions which take state as input and produce the next state as output. With this change, that's no longer true. They take the state + the internal state object as input instead. You may want to think through any assumptions of redux which might be violated by that.
There was a problem hiding this comment.
@wcjordan , good point. I've been debating myself over this the last couple of days...
Storing internal state outside the redux store almost makes it seem like it's an external factor to our reducers, which can make the reducers seem impure in a sense.
But I'll also argue that currently internal state values are strictly and purely calculated out of input state, and are not modifiable anywhere else.
So internal state could just be seen as functions over input state.
Technically, this makes our reducers remain pure and deterministic.
The only other issue that I could think of is how internal state is mutated...
This breaks time travel debugging and serialization of state, both of which we haven't been caring about since v1, but I was wondering if anything else breaks?
In our reducers, we assign parts of internal state back to output state (eg: slices of storedHeights are copied over to visible row heights).
From React's/Redux's perspective, output state could be seen as mutated, potentially causing strict equality checks to pass even when data changed.
But so far, this has never been an issue just because the entire subset of data that gets assigned over from internal state to output state is primitive data.
I think we're safe here for now, but we'll need to revisit this once more if our use cases start getting complicated.
There was a problem hiding this comment.
Got it. Thanks for your analysis. I appreciate the detail and sounds like we're on a reasonable track with this change.
Internal State is no longer a direct part of the redux store. It's instead accessed using a getter method `getInternal`. This is used to speed up code execution by avoiding overheads introduced by immer. Explanation: Immer internally uses proxies on the entire redux store state inorder to detect state mutations in reducers, but watching large data structures is inefficient and slows down reducers. With the changes in this commit, Internal state is no longer a direct part of the redux store state because we separated it through a getter. This means there's no proxies watching over the internal state, and hence mutating it has no overheads. Other changes: I also added a "Dynamic Row Heights" example.
|
Merged manually through 6989002. |
Context
Our redux store has a group of state fields known as "internal state".
These are only used within our reducers, but are mutated for performance.
Fields like
storedHeights(which stores the height for each row),rowBufferSet, androwOffsetIntervalTreeare part of the internal state.Starting from FDT v2 onwards, we use
reduxjs/toolkit, which in turn usesimmer.With
immer, our reducers no longer need to worry about immutability.Problem
Internally, immer uses proxies on the entire redux store state inorder to detect mutations.
This is slightly inefficient against large data structures (eg: an array containing 1 million elements).
Unfortunately, fields in "internal state", like
storedHeights, are pretty big most of the time and are expensive for immer.There's performance drops when we start modifying internal state every render, which is often the case when we scroll in a table having dynamic row heights.
Solution
We move out internal state out of the final redux store state, and instead expose it through a getter function
getInternal().The getter function has a closure on "internal state", and can easily return it.
This means there's no proxies watching over the actual internal state, and hence mutating it has no overheads.
Additional Changes
I also added a "dynamic row heights" example where a table with 100k rows with varying heights are rendered.
Without the fix, there's performance drops while vertically scrolling across the table.
How Has This Been Tested?
Tested in LD, and our existing examples.
Screenshots (if appropriate):
Types of changes
Checklist: