-
Notifications
You must be signed in to change notification settings - Fork 1.5k
indexeddb: improve allDocs() perf with skip & key ranges #8603
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
Conversation
|
@alxndrsn Awesome work! Thanks for the detailed reporting. Lots of big improvements, and plenty that are break even. The only one that really stands out is |
Great question. The test is defined at: So it's measuring |
|
Added new test for local docs with attachments at #8785; maybe useful for this PR as it will separate storage of local docs from other docs. |
garethbowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it.
There are quite a lot of TODO and REVIEW comments which should be addressed one way or the other.
Some ideas inline...
packages/node_modules/pouchdb-adapter-indexeddb/src/bulkDocs.js
Outdated
Show resolved
Hide resolved
packages/node_modules/pouchdb-adapter-indexeddb/src/bulkDocs.js
Outdated
Show resolved
Hide resolved
This reverts commit 8b95c7f.
Fixes error: An error occurred while bailing: TypeError: Cannot read property 'forEach' of undefined
Fixes error: An error occurred while bailing: TypeError: Cannot read property 'forEach' of undefined And subsequent error: An error occurred while bailing: TypeError: test.titlePath is not a function
SourceR85
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logical changes look good to me.
Would be nice, if @garethbowen could take a final look at these changes.
garethbowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just readability fixes - the rest looks great.
|
@alxndrsn Any idea why this change makes the views perform worse? |
Great question, looking into this. |
|
@garethbowen I've re-run perf test suites Branches used represent
It shows slowdown for this branch for What's weird is that the original results shared in the PR showed so much improvement in temp-views. I don't have a satisfactory explanation for that. The slowdown is cause for some concern, but perhaps it doesn't matter? Pouch docs read:
|
|
I agree that temp views aren't a big concern (at least for me!) but |
As above, I'm also confused by this. I think I have a separate fix which reduces the impact significantly... but also applies to the |
|
@garethbowen I've deleted the
|
|
garethbowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One naming suggestion, and two left over suggestions from a previous review, then I think this is ready to go.
garethbowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready when you are. Great work on this!
|
@garethbowen I've made a small update to Please note that this marks a change from the behaviour of the |
This improves
pouchdb-adapter-indexeddbperformance for key ranges by:[deleted, id]) to allow for skipping deleted docs by default_local/docs to theMETA_STOREto simplifyallDocs()implementationTODO
allDocs()_getLocal()implementationMETA_STOREtoMETA_LOCAL_STOREgetRevisionTree()/doCompaction()need to support local docsBenchmarks
The standard performance test suite was run with:
Client
Adapters
idb-pouchdb-adapter-idbas on the current master (30fd84f)master-pouchdb-adapter-indexeddbas on the current masterthis-pr-pouchdb-adapter-indexeddbas on this branchResults
The full suite was run 51 times, and the lowest median score for each test was taken for each adapter.
Key:
!.~.