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

Improve BuilderNode pooling significantly and add better comments / documentation #27

Closed
wants to merge 14 commits into from

Conversation

richardartoul
Copy link

@richardartoul richardartoul commented Jan 5, 2019

The existing implementation was missing a lot of places where BuilderNodes should be returned to the pool, which was causing the pool to leak massively and allocations would be very high when building FSTs. This addresses that issue, improves the comments / documentation, and also sets more reasonable default values for the pooling.

before

goos: darwin
goarch: amd64
pkg: github.com/m3db/vellum
BenchmarkBuilder-8           2000        649700 ns/op      269256 B/op        5832 allocs/op
PASS
ok      github.com/m3db/vellum    1.381s
Success: Benchmarks passed.

after

goos: darwin
goarch: amd64
pkg: github.com/m3db/vellum
BenchmarkBuilder-8   	    3000	    465007 ns/op	     737 B/op	       5 allocs/op
PASS
ok  	github.com/m3db/vellum	1.465s
Success: Benchmarks passed.

@sreekanth-cb
Copy link
Contributor

sreekanth-cb commented Mar 28, 2019

Hi @richardartoul,

Apologies for the delayed response, and really appreciate the upstream contribution efforts!

This is very cool! It's gonna take a bit to absorb what you did here, but the #'s are really exciting!

One other thing is, we actually do code reviews over in another system, called Gerrit (same as what android, chrome, golang, libreoffice and others use).

It's running here... http://review.couchbase.org/ and here's the vellum drill down: http://review.couchbase.org/#/q/project:vellum

One remaining item is that we ask that contributors sign the Couchbase CLA, see https://github.com/couchbaselabs/vellum/blob/master/CONTRIBUTING.md

We shall get this reviewed, verified and get is merged.

Request you follow the same process steps for the other PR as well.

@mschoch
Copy link
Contributor

mschoch commented Feb 12, 2021

Closing this now, as it is covered by #28

@ceejatec ceejatec closed this Feb 12, 2021
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.

4 participants