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 performance of HAMTs #4889

Merged
merged 4 commits into from
Apr 8, 2018
Merged

improve performance of HAMTs #4889

merged 4 commits into from
Apr 8, 2018

Conversation

Stebalien
Copy link
Member

According to the go benchmarks (not testing the datastore, etc.), these changes
make iterating over HAMTs ~50x faster and creating them ~2x faster.

based on #4884, merge after.

@Stebalien Stebalien requested a review from Kubuxu as a code owner March 29, 2018 02:13
@ghost ghost assigned Stebalien Mar 29, 2018
@ghost ghost added the status/in-progress In progress label Mar 29, 2018
t.Fatal("expected 1 bit set")
}

bint := new(big.Int).SetBytes(bf.Bytes())
Copy link
Member Author

Choose a reason for hiding this comment

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

Make sure we get the same thing as before.

// Before returns the number of bits set *before* this bit.
func (bf bitfield) Before(i int) int {
idx, off := bf.offset(i)
cnt := bits.OnesCount8(bf[idx] & (0xFF >> (8 - off)))
Copy link
Member Author

Choose a reason for hiding this comment

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

So, technically, we could make this faster with some unsafe magic to walk over 8 bytes at a time (casting them to a single uint64). However, I'm not sure if that'll help all that much (and this is going to be faster than anything we do with bigints).

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM but I would feel much better if @whyrusleeping looked at it too.

@Stebalien Stebalien force-pushed the feat/faster-hamt branch 2 times, most recently from 3e05b77 to 30e42e9 Compare March 30, 2018 01:19
License: MIT
Signed-off-by: Steven Allen <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
1. Use a custom bitfield type instead of bigints.
2. Make iterating over a hamt *significantly* faster.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
Copy link
Member Author

I've split the bitfield out into a separate package.

@whyrusleeping
Copy link
Member

TIL: go's big int implementation's Bytes() and SetBytes() method will not accurately round-trip a negative value.

@whyrusleeping
Copy link
Member

@Stebalien I wonder if you would get a performance improvement by using wordsize uint's instead of bytes, like the big int lib does.

@Stebalien
Copy link
Member Author

I wonder if you would get a performance improvement by using wordsize uint's instead of bytes, like the big int lib does.

Yeah, I probably should. Originally, I was worried about the cost of converting to/from bytes and planned on doing this with an unsafe cast but that's probably unnecessary.

@Stebalien
Copy link
Member Author

I wonder if you would get a performance improvement by using wordsize uint's instead of bytes, like the big int lib does.

So, it doesn't help with indexing but it does help significantly with counting bits. Unfortunately, it makes the conversion code messy as hell. I seel if I can clean it up a bit.

@Kubuxu
Copy link
Member

Kubuxu commented Apr 5, 2018

@Stebalien are you switching it to 64bit or are we leaving it as it is?

@Stebalien
Copy link
Member Author

I've written several patches to do so but they stink. I'd rather merge this and fix that later (as this patch has better performance regardless).

@Kubuxu Kubuxu added RFM and removed status/in-progress In progress labels Apr 6, 2018
@whyrusleeping whyrusleeping merged commit 585d97f into master Apr 8, 2018
@whyrusleeping whyrusleeping deleted the feat/faster-hamt branch April 8, 2018 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants