Skip to content

Conversation

@nolanlawson
Copy link
Member

@nolanlawson nolanlawson commented Dec 22, 2016

I rebased this on #6056 to avoid bitrotting myself.

This extends our use of getAll()/getAllKeys() to allDocs(), in addition to the work already done to use it in changes(). This was a bit trickier because the two have slightly different use cases, but this is documented in the code comments for runBatchedCursor.js.

There is an additional optimization we can do here where we avoid getAllKeys() entirely for allDocs() (it's not necessary, since unlike the by-seq-store for the doc-store we can derive the key from the value; it's not an autoincrementing key). However I'll save that for a future PR.

edit: I have a new implementation using batched cursors or a regular getAll depending on the scenario.

Benchmarks are TBD but in principle this is faster with no increased memory costs since we're still keeping the batch size limited based on limit.

@nolanlawson
Copy link
Member Author

nolanlawson commented Dec 24, 2016

Interestingly this is a perf boost relative to master in Firefox, but a regression in Chrome (chrome 119ms -> 868ms, firefox 496ms -> 157ms for an allDocs() test I'm running). However, that's counting the removal of the docCount optimization as well.

edit: it's a perf boost in Chrome as well if you consider the cost of the docCount optimization removal; I measured 1154ms for this particular test vs 868 with the getAll implementation.

@nolanlawson
Copy link
Member Author

Hm, based on these recent results I'm starting to suspect that objectStore.count() is just expensive in Chrome whereas it's cheap in Firefox (something to do with LevelDB vs SQLite?). In any case we may want to bring the docCount optimization back, but have it inside the store instead of in-memory so that it works across threads.

I am still happy with this PR but I'm sad that the docCount optimization helped so much in Chrome.

@nolanlawson
Copy link
Member Author

nolanlawson commented Dec 24, 2016

OK, in order to compare apples to apples, I compared using the new all-docs-startkey-endkey test using just the commits in this PR (i.e. using #6056 as the baseline), also bumped up to 50 iterations in order to be more sure):

  • Chrome: 6740ms -> 7583ms
  • Firefox: 2933ms -> 1092ms

So oddly it is a perf regression in Chrome. Apparently it's faster to fetch 10 documents using a cursor than to fetch them all at once using a single getAll().

This is a weird result, but I still think this is a good PR. We can improve Chrome perf by doing #6068. Also even if it's a bit slower, I can see from Dev Tools timelines that it spends less time on the UI thread because it has fewer callbacks. And a different benchmark may give a different result, e.g. a batch size of 50 instead of 10.

@nolanlawson
Copy link
Member Author

Small bug in descending=true mode that was only being tested in the mapreduce tests... so I added integration tests for it and fixed it. Should be green now.

@nolanlawson
Copy link
Member Author

Collapsed commits so it's easier to read.

Copy link
Contributor

@daleharvey daleharvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase but looks great, thanks

@nolanlawson
Copy link
Member Author

Rebased, will wait for a green before merging.

@nolanlawson
Copy link
Member Author

Green!

@nolanlawson nolanlawson merged commit b363f7d into master Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants