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

Performance improvements to iteration #1

Closed
wants to merge 4 commits into from
Closed

Performance improvements to iteration #1

wants to merge 4 commits into from

Conversation

cstockton
Copy link

Created a few more iterators to be used by the public interface for performance improvements. I did this in two commits, the second one changes the private interface signatures from funcs (technically same signature as ItemIterator) to Item, but since the public interface never actually exposes funcs for ascending I figured this would be okay.

Before:

~/.../github.com/cstockton/gbtree$ go test -test.bench=.*
1
PASS
BenchmarkInsert  3000000           411 ns/op
BenchmarkDelete  3000000           424 ns/op
BenchmarkGet     5000000           363 ns/op
BenchmarkIterateAscend      3000        474049 ns/op
BenchmarkIterateAscendLessThan      5000        274409 ns/op
BenchmarkIterateAscendGreaterOrEqual        5000        277208 ns/op
BenchmarkIterateAscendRange    10000        216873 ns/op
ok      github.com/cstockton/gbtree 16.429s

After:

~/.../github.com/cstockton/btree$ go test -test.bench=.*
1
PASS
BenchmarkInsert  3000000           408 ns/op
BenchmarkDelete  3000000           422 ns/op
BenchmarkGet     5000000           360 ns/op
BenchmarkIterateAscend      3000        431754 ns/op
BenchmarkIterateAscendLessThan      5000        257723 ns/op
BenchmarkIterateAscendGreaterOrEqual        5000        257560 ns/op
BenchmarkIterateAscendRange    10000        205764 ns/op
ok      github.com/cstockton/btree  15.984s

for i, item := range n.items {
if !from(item) {
if item.Less(from) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use n.items.find here to speed this up a bit? Now that you're looking for an item, no reason we can't binary-search for it.

@gconnell
Copy link
Collaborator

When you get a chance, could you sign the Google CLA? It's available at https://cla.developers.google.com/about/google-individual, and it needs to be signed before I accept code from you. Basically, it says "you still own the code, but you allow us to use it".

@cstockton
Copy link
Author

I signed it and associated it to this github account.

@gconnell
Copy link
Collaborator

Thanks for signing! I'll wait for your binary search changes before merging.

@cstockton
Copy link
Author

Hello, sorry was traveling over the last week but I just made the change you requested. Yielded a fair gain which of course favors larger degrees.

BenchmarkIterateAscendGreaterOrEqual       10000        230029 ns/op
BenchmarkIterateAscendRange    10000        183623 ns/op

That said, I see a few other areas for improvement that would be pretty simple to do but would be larger changes. For example I saw that every single node contains a reference to BTree which is only used to freeNode and newNode. Simply moving them to package level funcs allows the reference to the tree to be removed.. which yields some more gains:

BenchmarkInsert  5000000           394 ns/op
BenchmarkDelete  3000000           409 ns/op
BenchmarkGet     5000000           355 ns/op
BenchmarkIterateAscend      3000        430221 ns/op
BenchmarkIterateAscendLessThan      5000        255032 ns/op
BenchmarkIterateAscendGreaterOrEqual       10000        231429 ns/op
BenchmarkIterateAscendRange    10000        182023 ns/op
ok      github.com/cstockton/btree-find 17.269s

With the only use of t* removed, we could take out the reference to BTree entirely- which opens us up to allowing a new pointer to be introduced into each node without incurring any net-loss.. It might be nice to have each node reference the parent node to allow a morris traversal which I believe would be much faster. Just some food for thought.

EDIT:
Should I close this out?

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