Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.2k
Improve sorted entity adapter sorting performance#4361
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Review or Edit in CodeSandboxOpen the branch inWeb Editor •VS Code •Insiders |
✅ Deploy Preview forredux-starter-kit-docs ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
codesandbox-cibot commentedApr 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This pull request is automatically built and testable inCodeSandbox. To see build info of the built libraries, clickhere or the icon next to each commit SHA. Latest deployment of this branch, based on commit0ca3c60:
|
github-actionsbot commentedApr 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
size-limit report 📦
|
netlifybot commentedApr 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview forredux-starter-kit-docs ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
Uh oh!
There was an error while loading.Please reload this page.
markerikson commentedApr 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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): functionmerge(models:readonlyT[],state:R):void{constentities=state.entitiesasRecord<Id,T>constoldEntities=Object.values(state.ids).map((id:Id)=>entities[id])constnewEntities=models.slice().sort(sort)// Insert/overwrite all new/updatedmodels.forEach((model)=>{entities[selectId(model)]=model})constnewSortedIds:Id[]=[]leto=0,n=0while(o<oldEntities.length&&n<newEntities.length){constoldEntity=oldEntities[o]asT,oldId=selectId(oldEntity),newEntity=newEntities[n]// old entity has been overwritten by new entity, skip comparisonif(entities[oldId]!==oldEntity){o++continue}constcomparison=sort(oldEntity,newEntity)if(comparison<0){newSortedIds.push(oldId)o++continue}constnewId=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 to428bc2fCompare9e38eb3 tobece99bCompareCurrent benchmark numbers: Existing logic: Binary Insertion / this PR: I haven't tried to do any other perf profiling to measure where therest 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. |
markerikson commentedApr 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Latest times with additional Immer-related optimizations: The overhead seems to primarily be in Immer at this point: |
f94fc16 tod60cf84CompareThisshould be an improvement, let's get it out. |
unadlib commentedMay 14, 2024
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: constinitialItems=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)=>{constid=nanoid();draft.entity.ids.push(id)draft.entity.entities[id]={ id,position:0.9998,name:'test',}})})measureComparisons('Use only Mutative',()=>{create(store.getState(),(draft)=>{constid=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 ininsertion order, not the most recentsorted 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 notfast, but it's a 50% improvement.
Imay have another idea or two around tracking updated items, removing those, and re-inserting them, but will need to try those later.