Skip to content
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

(#3211) - remove unused metadata.seq index (idb) #3211

Closed
wants to merge 1 commit into from
Closed

Conversation

nolanlawson
Copy link
Member

So this is a bit of a hard choice, because if we ever want to bring this index back, it will have to be in a migration.

Besides the perf numbers below (which are good), my reasoning is thus:

I already tested using the metadata.seq for changes(), under the hypothesis that it would be better to only iterate winning seqs. In fact the perf was actually worse for temp-views, presumably because the cost of using the secondary index outweighed the benefit of increasing the read perf, which makes sense, since in those tests, all the docs are generation-1 and so there are no seqs to be skipped.

So there is an argument to be made that for databases with lots of revisions to a small number of documents, it would be faster to iterate over metadata.seq than seq. However, since most databases only need to use changes() for replication, which uses checkpoints, meaning it only has to iterate once, I think this is fine.

Plus, writes are slower than reads in LevelDB under the hood, and writes are what we want to really optimize for. Our read performance seems good, in general.

Anyway, perf numbers for this commit are below. Note that I bumped up the number of iterations for the insert tests in order to get more granularity:

browser test iterations before (ms) after (ms) improvement
Chrome 39 basic-inserts 10000 34749 28502 17.9774957553%
Firefox 36 basic-inserts 10000 85217 78201 8.23309902954%
Chrome 39 bulk-inserts 1000 60545 55115 8.96853579982%
Firefox 36 bulk-inserts 1000 152519 133135 12.7092362263%
Chrome 39 temp-views 1 11046 10984 0.561289154445%
Firefox 36 temp-views 1 19319 19041 1.43899787774%

@daleharvey
Copy link
Member

Great thanks - 3de1a20

@daleharvey daleharvey closed this Dec 19, 2014
@nolanlawson nolanlawson mentioned this pull request Dec 27, 2014
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.

None yet

2 participants