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

Optimize serialization #34

Merged
merged 1 commit into from
Jul 2, 2015
Merged

Conversation

bpot
Copy link
Collaborator

@bpot bpot commented Jun 30, 2015

Avoid using binary.Write which incurs non-trivial CPU and memory overhead. Improvements are more pronounced for bitmaps with more containers. This makes the serialization code a little less friendly but seems worth it.

Benchmarks

benchmark                        old ns/op     new ns/op     delta
BenchmarkSerializationSparse     2198058       350539        -84.05%
BenchmarkSerializationMid        666467        163984        -75.40%
BenchmarkSerializationDense      71531         42038         -41.23%

benchmark                        old allocs     new allocs     delta
BenchmarkSerializationSparse     18319          1528           -91.66%
BenchmarkSerializationMid        1843           155            -91.59%
BenchmarkSerializationDense      43             5              -88.37%

benchmark                        old bytes     new bytes     delta
BenchmarkSerializationSparse     445927        152896        -65.71%
BenchmarkSerializationMid        169706        140272        -17.34%
BenchmarkSerializationDense      25400         24720         -2.68%

@lemire
Copy link
Member

lemire commented Jun 30, 2015

This looks huge!

Currently away, will review soon.

@lemire
Copy link
Member

lemire commented Jul 2, 2015

Ok. I have reviewed the code. As far as I can tell, you are doing exactly what the golang library should be doing, but with a bit less overhead (that should not matter). I am not exactly sure why you see such huge gains... the speed should be close. If you have an explanation, please share.

In any case, one cannot argue with benchmarks so we are merging, obviously.

Note that such a low-level implementation could backfire if the golang library improves. But I have checked that the latest source code is just as naïve: https://golang.org/src/encoding/binary/binary.go

lemire added a commit that referenced this pull request Jul 2, 2015
@lemire lemire merged commit ed72110 into RoaringBitmap:master Jul 2, 2015
@bpot
Copy link
Collaborator Author

bpot commented Jul 2, 2015

I think a large part of the speed up is avoiding the trip through interface{} in binary.Write which burns CPU cycles and allocates a non-trivial number of objects.

I don't expect the current implementation to be improved too much. The docs mention it's designed for convenience over speed. Maybe, we'd see an improvement if they found a way to optimize some of the costs of using interface{}.

@lemire
Copy link
Member

lemire commented Jul 2, 2015

@bpot

The bulk of the code being called looks like this :

   for i, x := range v {
    order.PutUint64(bs[8*i:], uint64(x))
   }

I understand that the underlying calls for the for loop might not be inlined.

But why would many objects be allocated?

@bpot
Copy link
Collaborator Author

bpot commented Jul 2, 2015

I think the most expensive part was actually writing all of the "header" values in roaringarray.go. This is why the improvement is so much larger for bitmaps with a lot of containers.

The changes to the array and bitmap containers did speed things up but no nearly as much.

@lemire
Copy link
Member

lemire commented Jul 3, 2015

@bpot

Makes sense...

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