Repository navigation
perf: Countmin performance improvements - #300
Conversation
bucket_index reduced each row's hash with a 64-bit `%`, which cost about as much as the hash itself on update and estimate. Use a mask when num_buckets is a power of two or Lemire's direct remainder when not. Both return `h1 % num_buckets`, so bucket placement and serialized bytes are unchanged. The stored multiplier adds 24 bytes to estimated_size.
pmcgleenon
changed the title
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
Thanks! One minor comment inline.
There was a problem hiding this comment.
Will this perf improvement subtly change the estimation but still within the margin of error?
I think we may assert against the range.
There was a problem hiding this comment.
I have reviewed the logic here, and found that:
The reduction is exact, not approximate: fast_remainder and the power-of-two mask both return exactly what % returned, so bucket placement, estimates, and error bounds are bit-for-bit unchanged.
test_fast_remainder_matches_remainder already cross-checks this against % over the full supported divisor range.
There was a problem hiding this comment.
thanks for the review, yes the intent is that it will give the same result as before
There was a problem hiding this comment.
Oops. This is estimated_size not estimate 🤣
| /// Multiplier for [`fast_remainder`], derived from `num_buckets`. | ||
| bucket_multiplier: u128, |
There was a problem hiding this comment.
I personally find this field name strange, but I cannot find a better alternative.
I can live with it anyway.
There was a problem hiding this comment.
Replaces the remainder operation with a faster implementation as described in #298