-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Improve sorted entity adapter sorting performance #4361
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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. Latest deployment of this branch, based on commit 0ca3c60:
|
size-limit report 📦
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Revised numbers if we reset the sort counter right before the upsert:
|
Just leaving this here - I suspect that pre-sorting the incoming array and manually merging them might save a bunch of comparisons on top (assuming that we don't only insert at the end): function merge(models: readonly T[], state: R): void {
const entities = state.entities as Record<Id, T>
const oldEntities = Object.values(state.ids).map((id: Id) => entities[id])
const newEntities = models.slice().sort(sort)
// Insert/overwrite all new/updated
models.forEach((model) => {
entities[selectId(model)] = model
})
const newSortedIds: Id[] = []
let o = 0,
n = 0
while (o < oldEntities.length && n < newEntities.length) {
const oldEntity = oldEntities[o] as T,
oldId = selectId(oldEntity),
newEntity = newEntities[n]
// old entity has been overwritten by new entity, skip comparison
if (entities[oldId] !== oldEntity) {
o++
continue
}
const comparison = sort(oldEntity, newEntity)
if (comparison < 0) {
newSortedIds.push(oldId)
o++
continue
}
const newId = selectId(newEntity)
if (comparison > 0) {
newSortedIds.push(newId)
n++
continue
}
newSortedIds.push(oldId)
o++
newSortedIds.push(newId)
n++
}
while (o < oldEntities.length) {
newSortedIds.push(selectId(oldEntities[o]))
o++
}
while (n < newEntities.length) {
newSortedIds.push(selectId(newEntities[n]))
n++
}
if (!areArraysEqual(state.ids, newSortedIds)) {
state.ids = newSortedIds
}
} This was just a way around |
36e15d4
to
428bc2f
Compare
9e38eb3
to
bece99b
Compare
Current benchmark numbers: Existing logic:
Binary Insertion / this PR:
I haven't tried to do any other perf profiling to measure where the rest of the overhead is coming from. I'd guess that Immer is a large chunk of it. But still, that's a lot fewer comparisons for those cases. |
Latest times with additional Immer-related optimizations:
The overhead seems to primarily be in Immer at this point: |
f94fc16
to
d60cf84
Compare
This should be an improvement, let's get it out. |
I apologize for the sudden comment on this merged PR. Here's a performance comparison test between Immer and Mutative that focuses solely on "Insert One (end)."
Here's the test code: const initialItems = generateItems(INITIAL_ITEMS)
measureComparisons('Original Setup', () => {
store.dispatch(entitySlice.actions.upsertMany(initialItems))
})
measureComparisons('Insert One (end)', () => {
store.dispatch(
entitySlice.actions.upsertOne({
id: nanoid(),
position: 0.9998,
name: 'test',
}),
)
})
measureComparisons('Use only Immer', () => {
produce(store.getState(), (draft) => {
const id = nanoid();
draft.entity.ids.push(id)
draft.entity.entities[id] = {
id,
position: 0.9998,
name: 'test',
}
})
})
measureComparisons('Use only Mutative', () => {
create(store.getState(), (draft) => {
const id = nanoid();
draft.entity.ids.push(id)
draft.entity.entities[id] = {
id,
position: 0.9998,
name: 'test',
}
})
}) I know this performance test is quite specific. If you have any further ideas or questions, I’d be more than happy to discuss them. |
This PR:
Previously we were always getting the list of items via
Object.values(state.entities)
. However, that is always going to give us an array of items in insertion order, not the most recent sorted order.If we instead go
state.ids.map(id => state.entities[id])
, we at least start with a sorted array.However, there's a couple tricky cases. If a set of updates replaced an item's ID, that throws things out of whack. It's easier to fall back to the old behavior. There are also apparently a couple test cases where we can end up with duplicate IDs and have to watch for that.
On my own machine, the perf test case with 100K items gave me numbers like this with the existing code:
After this fix, it drops to:
That's still not fast, but it's a 50% improvement.
I may have another idea or two around tracking updated items, removing those, and re-inserting them, but will need to try those later.