Use a sync.Pool for bitmapContainers used in ToBytes()

[kevinconaway]

One of our applications maintains a lot of small compressed bitmaps in memory and occasionally serializes them to disk. When they are serialized, we notice a large bump in memory allocations (and accompanying cpu/gc activity) due to creating a new *bitmapContainer each time.

This commit modifies roaringArray to use a sync.Pool of *bitmapContainer in writeTo() instead of creating a new one each time. Since the bitmapContainers can now be reused, a clear() method is also added to reset its state

@lemire please let me know if you have alternative thoughts here. The goal is to avoid allocating a new *bitmapContainer (which is really a [1024]uint64 and an int) every time we call ToBytes() on a compressed bit map

[coveralls]

Coverage increased (+0.05%) to 82.326% when pulling b127a89 on kevinconaway:kevinconaway/bitmapcontainer-pool into 4d53b29 on RoaringBitmap:master.

[coveralls]

Coverage increased (+0.05%) to 82.326% when pulling b127a89 on kevinconaway:kevinconaway/bitmapcontainer-pool into 4d53b29 on RoaringBitmap:master.

[lemire]

Looks reasonable to me with the minor caveat that we may hold on to a bitmap container in the pool needlessly for a very long time (forever).

[lemire]

Ping @alldroll ... or anyone... can we discuss this?

[lemire]

I'd like to get feedback from a few others...

[lemire]

A better solution would be avoid the allocation entirely...

[kevinconaway]

with the minor caveat that we may hold on to a bitmap container in the pool needlessly for a very long time (forever).

Prior to go 1.13, pools were cleared at the start of every GC

The behavior has changed since then but it seems to indicate that objects will still be collected from the pool:

but if Pool usage drops, objects will still be collected within two GCs (as opposed to one).

[kevinconaway]

A better solution would be avoid the allocation entirely...

Yes, agreed. I don't know enough about the internals to suggest a solution though.

[lemire]

@kevinconaway The notes regarding the GC is reassuring. This answers my main objection.

I will give people time to react...

[alldroll]

I have some thoughts regarding this issue:

Instead of initializing bitmapContainer, we can allocate memory on stack.
Here is my draft

func newBitmapContainerByValue() bitmapContainer {
	var bitmapBuf [(1 << 16) / 64]uint64

	return bitmapContainer{
		bitmap: bitmapBuf[:],
	}
}

And now we can call newBitmapContainerByValue instead of newBitmapContainer

func BenchmarkSerializationSequential(b *testing.B) {
	b.ReportAllocs()
	b.StopTimer()

	s := NewBitmap()
	initsize := 650000
	for i := 0; i < initsize; i++ {
		s.Add(uint32(i))
	}

	s.RunOptimize()

	buf := make([]byte, 0, s.GetSerializedSizeInBytes())
	b.StartTimer()

	for j := 0; j < b.N; j++ {
		w := bytes.NewBuffer(buf[:0])
		s.WriteTo(w)
	}
}

// old
BenchmarkSerializationSequential-8   	  545766	      1841 ns/op	    8416 B/op	      13 allocs/op

// proposed from PR
BenchmarkSerializationSequential-8   	  955692	      1159 ns/op	     224 B/op	      12 allocs/op

// my solution
BenchmarkSerializationSequential-8   	 1525407	       782 ns/op	     224 B/op	      12 allocs/op

What do you think about it?

[lemire]

+1 for the benchmark and the better performance, but I am thinking about something even simpler... let me see...

[lemire]

Can you guys review my proposal at #247

?

[kevinconaway]

Closing in favor of #247