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

configure stack size #3

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

configure stack size #3

wants to merge 18 commits into from

Conversation

mschoch
Copy link
Contributor

@mschoch mschoch commented Feb 12, 2021

No description provided.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 12, 2021

NOTE: this is a breaking change (adds exported field to exported struct), and once merging we will need toincrement the major version.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 12, 2021

This is a migration of: couchbase/vellum#28

@mschoch
Copy link
Contributor Author

mschoch commented Feb 12, 2021

Also @abhinavdangeti can you check CLA status, I no longer have permission to see that list.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 12, 2021

I reverted the change to the benchmark. First, generally when we want to compare before/after results it's unfair to change the benchmark in the same change. Second, I don't think it was an appropriate change anyway. To benchmark building a vellum, you should actually build it each time. Reusing the builder on subsequent iterations means we're not really doing the same work each loop through the benchmark.

That said, one still might argue that reuse is what real applications would do, so I'm going to propose we have 2 benchmarks, one reuses the builder, one does not. The first is more useful if you're actually trying to measure improvements to the building process. The second is more useful at measuring how well reusing the builder helps.

I will add this new benchmark to master and we can merge it in here, and see the results of both benchmarks before and after.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 18, 2021

OK, so here are the updated benchmark results (old is current master, new is this branch)

benchmark                           old ns/op      new ns/op      delta
BenchmarkBuilder-32                 1183670        1009201        -14.74%
BenchmarkBuilderReuse-32            888162         388331         -56.28%
BenchmarkFSTIteratorAllInMem-32     248951         243240         -2.29%
BenchmarkKey4000K-32                1285256251     1084494835     -15.62%
BenchmarkKey1000K-32                284561275      294392406      +3.45%
BenchmarkKey100K-32                 41436773       41918003       +1.16%
BenchmarkKey10K-32                  4332922        4351589        +0.43%
BenchmarkKey1K-32                   472138         469722         -0.51%

benchmark                           old allocs     new allocs     delta
BenchmarkBuilder-32                 5843           2656           -54.54%
BenchmarkBuilderReuse-32            3989           5              -99.87%
BenchmarkFSTIteratorAllInMem-32     42             42             +0.00%
BenchmarkKey4000K-32                12000060       12000060       +0.00%
BenchmarkKey1000K-32                2999986        2999991        +0.00%
BenchmarkKey100K-32                 299976         299977         +0.00%
BenchmarkKey10K-32                  29966          29966          +0.00%
BenchmarkKey1K-32                   2957           2957           +0.00%

benchmark                           old bytes     new bytes     delta
BenchmarkBuilder-32                 603216        468810        -22.28%
BenchmarkBuilderReuse-32            191244        609           -99.68%
BenchmarkFSTIteratorAllInMem-32     4024          4024          +0.00%
BenchmarkKey4000K-32                625620928     625622456     +0.00%
BenchmarkKey1000K-32                158527228     158527816     +0.00%
BenchmarkKey100K-32                 16292105      16292159      +0.00%
BenchmarkKey10K-32                  1847782       1847806       +0.00%
BenchmarkKey1K-32                   460640        460664        +0.01%

Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Thanks for getting this benchmarked @mschoch.
As we've reviewed the code already, and these numbers look mostly good - I'm ok to merge this.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 19, 2021

OK, so since this will be a new major version I wanted to go a little bit slower. Today I ran some more full-stack indexing building tests, to see if this helped in the real world. The test is using search-benchmark-game which indexes just over 5 million documents. All tests ran with bleve v2.0.0.

First, I ran 2 runs with stock bleve v2.0.0:

make index  2614.90s user 56.02s system 261% cpu 17:02.50 total
make index  2606.12s user 55.40s system 262% cpu 16:55.39 total

Then I used a go mod rewrite to point to this new vellum and ran 2 more runs:

make index  2544.45s user 51.79s system 260% cpu 16:37.36 total
make index  2555.52s user 54.23s system 260% cpu 16:42.42 total

So, some improvement, but nothing huge. Then, just to sanity check that there is some signal here and it isn't all noise. I switched it back to stock bleve v2.0.2 and ran it one more time:

make index  2605.23s user 53.94s system 262% cpu 16:54.79 total

So, my unscientific conclusion is that there is some real word benefit, but at the moment it isn't very large.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 19, 2021

Just to be a bit more scientific, converting to seconds for easier comparison:

old:

1022
1015
1014

new:

997
1002

Taking the fastest of the old (1014) and the slowest of the new (1002) we get a diff of 12 seconds, which is just around 1% of the ballpark times.

@abhinavdangeti
Copy link
Member

Hey @richardartoul , I don't see that you've signed the CLA to make contributions to the project. Would you follow instructions as listed here .. https://github.com/blevesearch/vellum/blob/master/CONTRIBUTING.md

Once you've done that, we can go ahead with merging this into the code base.

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