Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
markerikson merged 7 commits intomasterfromfeature/4252-entity-adapter-sorting
May 9, 2024

Conversation

@markerikson
Copy link
Collaborator

This PR:

  • Updates the sorting logic for sorted entity adapters to hopefully be more efficient most of the time

Previously we were always getting the list of items viaObject.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 gostate.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:

Upsert: sortComparer called: 3,058,124 timesDuration to upsert an item: 1,177 msUpdate: sortComparer called: 1,529,076 timesDuration to update an item: 1,139 ms

After this fix, it drops to:

Upsert: sortComparer called: 1,629,402 timesDuration to upsert an item: 533 msUpdate: sortComparer called: 100,032 timesDuration to update an item: 544 ms

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.

riqts and kcrwfrd reacted with heart emojikcrwfrd reacted with rocket emoji
@codesandbox
Copy link

Review or Edit in CodeSandbox

Open the branch inWeb EditorVS CodeInsiders

OpenPreview

@netlify
Copy link

netlifybot commentedApr 18, 2024

Deploy Preview forredux-starter-kit-docs ready!

NameLink
🔨 Latest commitd0b3ba5
🔍 Latest deploy loghttps://app.netlify.com/sites/redux-starter-kit-docs/deploys/66218b86f457630009970d92
😎 Deploy Previewhttps://deploy-preview-4361--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

@codesandbox-ci
Copy link

codesandbox-cibot commentedApr 18, 2024
edited
Loading

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:

SandboxSource
rsk-github-issues-exampleConfiguration
@examples-query-react/basicConfiguration
@examples-query-react/advancedConfiguration
@examples-action-listener/counterConfiguration
rtk-esm-craConfiguration

@github-actions
Copy link

github-actionsbot commentedApr 18, 2024
edited
Loading

size-limit report 📦

PathSize
1. entry point: @reduxjs/toolkit (cjs, production.min.cjs)14.32 KB (+1.56% 🔺)
1. entry point: @reduxjs/toolkit/react (cjs, production.min.cjs)14.44 KB (+1.52% 🔺)
1. entry point: @reduxjs/toolkit/query (cjs, production.min.cjs)22.18 KB (+0.93% 🔺)
1. entry point: @reduxjs/toolkit/query/react (cjs, production.min.cjs)24.16 KB (+0.9% 🔺)
2. entry point: @reduxjs/toolkit (without dependencies) (cjs, production.min.cjs)7.71 KB (+2.71% 🔺)
3. createEntityAdapter (.modern.mjs)5.4 KB (+3.86% 🔺)

@netlify
Copy link

netlifybot commentedApr 18, 2024
edited
Loading

Deploy Preview forredux-starter-kit-docs ready!

NameLink
🔨 Latest commit0ca3c60
🔍 Latest deploy loghttps://app.netlify.com/sites/redux-starter-kit-docs/deploys/663c22fbf1d8fb0008af9ca0
😎 Deploy Previewhttps://deploy-preview-4361--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

@markerikson
Copy link
CollaboratorAuthor

markerikson commentedApr 19, 2024
edited
Loading

Revised numbers if we reset the sort counter right before the upsert:

// 100K items, without fixUpsert: sortComparer called: 1,529,445 timesDuration to upsert an item: 1,379 msUpdate: sortComparer called: 1,529,444 timesDuration to update an item: 1,482 ms// 100K items, with fixUpsert: sortComparer called: 100,027 timesDuration to upsert an item: 847 msUpdate: sortComparer called: 100,032 timesDuration to update an item: 803 ms10K items, with fixUpsert: sortComparer called: 10,017 timesDuration to upsert an item: 92 msUpdate: sortComparer called: 10,024 timesDuration to update an item: 71 ms

@phryneas
Copy link
Member

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 aroundmerge which will only work for insert/upsert, but I believe my approach would also work with updating ids - it just needs the sorted entries from before, the merged object and the inserted/updated objects (also sorted). Could just as well be a method argument like you did.
Feel free to swipe anything if you feel it's a useful approach.

@markeriksonmarkeriksonforce-pushed thefeature/4252-entity-adapter-sorting branch from36e15d4 to428bc2fCompareApril 20, 2024 22:59
@markeriksonmarkeriksonforce-pushed thefeature/4252-entity-adapter-sorting branch from9e38eb3 tobece99bCompareApril 20, 2024 23:50
@markerikson
Copy link
CollaboratorAuthor

Current benchmark numbers:

Existing logic:

Original Setup: sortComparer called 1,529,087 times in 839msInsert One (random): sortComparer called 1,529,107 times in 1,018msInsert One (middle): sortComparer called 1,529,120 times in 1,311msInsert One (end): sortComparer called 1,529,137 times in 1,348msAdd Many: sortComparer called 1,546,947 times in 1,283msUpdate One (end): sortComparer called 1,546,945 times in 1,338msUpdate One (middle): sortComparer called 1,546,948 times in 1,375msUpdate One (replace): sortComparer called 1,547,010 times in 1,403ms

Binary Insertion / this PR:

Original Setup: sortComparer called 1,529,043 times in 642msInsert One (random): sortComparer called 17 times in 569msInsert One (middle): sortComparer called 16 times in 452msInsert One (end): sortComparer called 17 times in 487msAdd Many: sortComparer called 16,728 times in 519msUpdate One (end): sortComparer called 101,033 times in 590msUpdate One (middle): sortComparer called 101,035 times in 586msUpdate One (replace): sortComparer called 101,055 times in 700ms

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
Copy link
CollaboratorAuthor

markerikson commentedApr 22, 2024
edited
Loading

Latest times with additional Immer-related optimizations:

Original Setup: sortComparer called 1,529,339 times in 600msInsert One (random): sortComparer called 16 times in 349msInsert One (middle): sortComparer called 16 times in 441msInsert One (end): sortComparer called 16 times in 377msAdd Many: sortComparer called 16,710 times in 384msUpdate One (end): sortComparer called 101,034 times in 550msUpdate One (middle): sortComparer called 101,036 times in 492msUpdate One (replace): sortComparer called 101,055 times in 655ms

The overhead seems to primarily be in Immer at this point:

image

@markerikson
Copy link
CollaboratorAuthor

Thisshould be an improvement, let's get it out.

@markeriksonmarkerikson merged commit7314c49 intomasterMay 9, 2024
@unadlib
Copy link

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)."
The results are as follows:

// 10K itemsOriginal Setup: sortComparer called 119,925 times in 40msInsert One (end): sortComparer called 13 times in 12msUse only Immer: sortComparer called 0 times in 10msUse only Mutative: sortComparer called 0 times in 2ms// 100K itemsOriginal Setup: sortComparer called 1,529,198 times in 349msInsert One (end): sortComparer called 17 times in 165msUse only Immer: sortComparer called 0 times in 120msUse only Mutative: sortComparer called 0 times in 24ms// 1000K itemsOriginal Setup: sortComparer called 18,604,782 times in 3,559msInsert One (end): sortComparer called 19 times in 2,859msUse only Immer: sortComparer called 0 times in 1,787msUse only Mutative: sortComparer called 0 times in 368ms

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.

its-monotype reacted with thumbs up emoji

@aryaemami59aryaemami59 deleted the feature/4252-entity-adapter-sorting branchJanuary 3, 2025 07:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@phryneasphryneasphryneas left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@markerikson@phryneas@unadlib

[8]ページ先頭

©2009-2025 Movatter.jp