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

[Draft] Add support for custom allocators #366

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Sep 8, 2022

TODO: Its error prone to require allocator to be non-nil. Could just do a nil check on each usage so that users instantiating bitmap like &Bitmap{} aren't broken. Will address if we decide to move forward with this approach.

Benchmark Before

goos: darwin
goarch: amd64
pkg: github.com/RoaringBitmap/roaring
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkRepeatedSparseSerialization-8   	 3732640	       316.4 ns/op	      96 B/op	       5 allocs/op
PASS
ok  	github.com/RoaringBitmap/roaring	1.849s

Benchmark After

goos: darwin
goarch: amd64
pkg: github.com/RoaringBitmap/roaring
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkRepeatedSparseSerialization-8   	 5236890	       220.4 ns/op	      24 B/op	       1 allocs/op
PASS
ok  	github.com/RoaringBitmap/roaring	1.575s

@lemire
Copy link
Member

lemire commented Sep 8, 2022

You go from 316.4 ns/op to 220.4 ns/op. Can you provide a rationale as to why your code (which defers to the Go make function) is so much faster?

@richardartoul
Copy link
Contributor Author

@lemire It's not defering to the Go make function I'm pretty sure (I wrote this quickly, totally possible I made a mistake). See the difference in allocations:

Before

 5 allocs/op

After

1 allocs/op

The reason for this is that the following branches are always true:

if size <= cap(a.buf) && capacity <= cap(a.buf) {

and

if size <= cap(a.uint16s) && capacity <= cap(a.uint16s) {

and I only have a single allocator so its just reusing the single instance of buf and uint16s over and over again instead of creating new ones constantly. Once l.WriteTo() is done, all the data has been captured in the output buffer and its safe to reuse the same instances for the next iteration. Thats the idea anyways!

@richardartoul
Copy link
Contributor Author

(The benchmark is using a custom allocator that always returns the same slices basically, which is an overly simplified version of what we would do in our production code as well)

@richardartoul
Copy link
Contributor Author

richardartoul commented Sep 8, 2022

@lemire Ah ok I think I see the source of the confusion. My proposal is not to implement a "default" thread-safe allocator for everyone to use by default. My proposal is to support users injecting their own allocator, and users will write whatever type of allocator makes sense for them. They could write an allocator that uses manual refcounting and is globally shared with locks, or they can do what I plan to do which is to spawn a dedicated allocator in critical path single-threaded loops that allows each iterator of a loop to reuse datastructures over and over again.

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