perf(internal/bits): Significantly speedup bitArray.PickRandom

[ValarDragon]

This PR significantly speeds up bitArray.PickRandom which is used in VoteGossip and BlockPart gossip. We saw for a query serving full node, over an hour, this was a very large amount of RAM allocations. (75GB of RAM!)

This PR drops it down to 0 allocations, and makes the routine 10x faster on my machine.

OLD:

BenchmarkPickRandomBitArray-12           1545199               846.1 ns/op          1280 B/op          1 allocs/op

NEW:

BenchmarkPickRandomBitArray-12          22192857                75.39 ns/op            0 B/op          0 allocs/op

I think the new tests I wrote make this more tested than the old code that was here tbh, but pls let me know if theres more tests we'd like to see!

---

PR checklist

• Tests written/updated
• Changelog entry added in .changelog (we use unclog to manage our changelog)
• Updated relevant documentation (docs/ or spec/) and code comments
• Title follows the Conventional Commits spec

[melekes]

Thanks @ValarDragon ❤️

[cason]

Great work.

Left some comments regarding the documentation of the introduced methods.

[cason]

Returns -1, doesn't it?

[ValarDragon]

Whoops yes, thank you!

[cason]

This is requirement is not really a requirement, right? The code support this case, but returns -1.

[ValarDragon]

True!

[cason]

I didn't add to the review, but I was thinking the following. This is only a suggestion.

We implement PickRandom() by 1. finding all bits set 1/true, lets say n, then 2. randomly taking one of these bits (random from 0 to n-1). So, we need 1. n to produce the random number used in 2.

Assuming that we know the maximum number of bits stored, lets say m, why we don't pick a random value from 0 to m, lets say i. Of course, probably in most cases, we have m > n, so potentially i > n. But in this case, actually in the general case, we just need to pick the index i % n. The problem, of course, is that we don't know n a priori (which can be solved in a different way, but lets keep this a bit simpler).

My point here is that we probably can implement PickRandom() with a single method. Of course, this method would need another loop, to handle the case where i > n. But in this case, after the first loop, we know n and we know that i > n. Therefore can compute i % n, and solve the problem in the second iteration.

[cason]

I wonder if adding some contention to this benchmark would affect the results. I think the results are better with this PR, but maybe this is something to explore. By contention, I mean, another thread changing the bit set while we are retrieving random indexes.

[ValarDragon]

Where this is used, these mutexes are truly useless. We are operating on a clone that is only owned in a single threaded function. Was going to make an issue to make a new bit array version that gets rid of the mutexes

[cason]

This would be ideal. In particular because I assume that the methods using this are already synchronized.

For instance, consensus state, peer consensus state should be synchronized.

[ValarDragon]

I thought about the second method, but getting perfect distribution will require rejection sampling which is potentially much slower. (Doing i%n biases the random distribution which is more scary for edge case slowdowns imo.)

Rejection sampling seems like unneeded edge case slowdowns right now. Targeting a single pass seems like large overkill when in the systemic view or what's going on with the bit arrays, we're very wasteful at the moment. (Two Extra full clones in the relevant vote sampling code. We clone the bit array from votes, then do a sub call that allocates another bit array, and everything here is single threaded but does mutexes. The mutex call's Cas operation is likely slower than the first pass of this codes ~16 bitwise math ops per word)

[cason]

I didn't fully get this rejection sampling part, but I was wondering whether i % n is as random as rand(n), but I am not sure. Maybe it is not, but probably if we make m big enough, it becomes pretty much as random as.

[cason]

Also agree that removing locks is much more relevant for performance than saving a few loops.

[cason]

The problem, of course, is that we don't know n a priori (which can be solved in a different way, but lets keep this a bit simpler).

We can have this as a private field:

• SetIndex adds or subtracts 1
• copy() can be adapted easily
• IsEmpty() and IsFull() become trivial
• Update(), I don't even know the point, same as copy()
• UnmarshalJSON() uses SetIndex, so nothing new here
• FromProto() can have this field initialized with a private method (the one added here)

I should probably create an issue from this.

[ValarDragon]

Your totally right, that would work! I expect that the difference in performance for the PickRandom call won't be that high (I think were already fast enough here / remaining time is coming from the Mutex), but agreed that seems like a better solution!