-
-
Notifications
You must be signed in to change notification settings - Fork 429
Add BloomTokenLog
#2136
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
base: main
Are you sure you want to change the base?
Add BloomTokenLog
#2136
Conversation
f96e7c5
to
9d19830
Compare
0b4e414
to
d855d1f
Compare
@djc and @Ralith, some meta-discussion I'd like to explicitly invite, and this applies to #2137 as well: It may be worth evaluating whether we want to go this route in the first place (of adding these defaults). It's possible we decide we want to go a different route. Some alternatives could include:
Points in favor of merging these PRs:
Points against merging these PRs:
I think it's worth putting these in here, but I'd like to hear what you think. |
re: meta-discussion: I think I'm on board with including this functionality out of the box. It seems to strike a good balance: typical users won't be affected by a 20MiB worst-case memory use increase, and will benefit from better handshake performance. Conversely, anyone who can't afford that probably already needs to customize e.g. flow control limits, so a little more tuning is not much to ask of them. As far as the dep goes, I think a third-party dep is less maintenance load for us than becoming experts in bloom token implementation. That said, I did feel obligated to have a brief skim, and unfortunately immediately found UB. Let's consider that a blocker for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review with fresh eyes.
d855d1f
to
e6b0565
Compare
- Adds default feature `bloom`. - It enables a new BloomTokenLog implementation of TokenLog. - It enables a dependency on the `fastbloom` crate.
Unless the fastbloom feature is disabled, in which case it still falls back to NoneTokenLog.
It makes sense to do this now that we have a default token log that's actually able to accept tokens.
e6b0565
to
3fc5f81
Compare
Tests no longer reconfigure config to use SimpleTokenLog if the fastbloom feature is enabled, instead preferring to use the default BloomTokenLog.
3fc5f81
to
e487849
Compare
aws-lc-rs = ["dep:aws-lc-rs", "aws-lc-rs?/aws-lc-sys", "aws-lc-rs?/prebuilt-nasm"] | ||
aws-lc-rs-fips = ["aws-lc-rs", "aws-lc-rs?/fips"] | ||
# Enable things that use bloom filter dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation isn't very informative. How about something like "Enables BloomTokenLog
, and uses it by default"
@@ -498,7 +507,8 @@ impl ValidationTokenConfig { | |||
/// | |||
/// This refers only to tokens sent in NEW_TOKEN frames, in contrast to retry tokens. | |||
/// | |||
/// Defaults to 0. | |||
/// If the `fastbloom` feature is enabled (which it is by default), defaults to 2. Otherwise, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If the `fastbloom` feature is enabled (which it is by default), defaults to 2. Otherwise, | |
/// If the `bloom` feature is enabled (which it is by default), defaults to 2. Otherwise, |
fastbloom
BloomTokenLog
, a newTokenLog
implementation:fastbloom
crate is a popular bloom filter implementation with SIMD acceleration.