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

investigate: ristretto cache with memory bounding #508

Closed
wants to merge 3 commits into from

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jun 18, 2022

This is a follow-up to: #506 (comment)
with an attempt to bring in a ristretto cache instead of using a custom implementation. This PR documents the progress and findings and should not be merged in its current state.

This PR only swaps the fast node cache to the ristretto cache.
The following is the config:

NumCounters: 1e7,     // number of keys to track frequency of (10M).
MaxCost:     1 << 27, // maximum cost of cache (approx 130MB).
BufferItems: 64,      // number of keys per Get buffer. (suggested default value)

With the above config, the cache is supposed to be capped at 130 MB.

There were 2 issues with the switch to ristretto:

  • some tests started failing for unknown reasons
    • According to this article, ristretto is susceptible to key collisions. I suspect that might be the reason why
  • degraded performance:
name                                                                  old time/op    new time/op    delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-16    2.80µs ± 3%    3.29µs ± 6%  +17.32%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-16    9.62µs ± 5%    9.30µs ± 4%     ~     (p=0.056 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-fast-16                     338ns ± 2%     395ns ± 6%  +16.86%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-slow-16                    14.3µs ± 2%    13.5µs ± 2%   -5.87%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-fast-16                     64.4ms ± 2%    58.3ms ± 2%   -9.54%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-slow-16                      1.21s ±13%     1.15s ±12%     ~     (p=0.095 n=5+5)
Medium/goleveldb-100000-100-16-40/update-16                              180µs ±24%     171µs ±16%     ~     (p=0.310 n=5+5)
Medium/goleveldb-100000-100-16-40/block-16                              24.5ms ± 5%    22.5ms ± 9%     ~     (p=0.095 n=5+5)

name                                                                  old alloc/op   new alloc/op   delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-16      790B ± 0%      822B ± 0%   +4.05%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-16    1.41kB ± 1%    1.41kB ± 1%     ~     (p=0.460 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-fast-16                     0.00B         31.00B ± 0%    +Inf%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-slow-16                    2.02kB ± 1%    2.03kB ± 0%   +0.49%  (p=0.040 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-fast-16                     29.3MB ± 0%    29.3MB ± 0%   -0.00%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-slow-16                      276MB ± 0%     277MB ± 0%   +0.36%  (p=0.029 n=4+4)
Medium/goleveldb-100000-100-16-40/update-16                             51.3kB ± 8%    51.7kB ± 7%     ~     (p=1.000 n=5+5)
Medium/goleveldb-100000-100-16-40/block-16                              6.50MB ± 6%    6.50MB ± 5%     ~     (p=1.000 n=5+5)

name                                                                  old allocs/op  new allocs/op  delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-16      15.0 ± 0%      16.0 ± 0%   +6.67%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-16      24.0 ± 0%      24.0 ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-hits-fast-16                      0.00           1.00 ± 0%    +Inf%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-slow-16                      35.0 ± 0%      35.0 ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/iteration-fast-16                       523k ± 0%      523k ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/iteration-slow-16                      4.71M ± 0%     4.71M ± 0%   +0.05%  (p=0.029 n=4+4)
Medium/goleveldb-100000-100-16-40/update-16                                561 ±16%       567 ±15%     ~     (p=1.000 n=5+5)
Medium/goleveldb-100000-100-16-40/block-16                               73.2k ± 4%     72.5k ± 5%     ~     (p=0.690 n=5+5)

I suspect that is because ristretto provides thread safety and concurrency guarantees that we might not really need for our use case.

Based on these findings, I don't think we should investigate ristretto further at this time

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 18, 2022

This pull request introduces 1 alert when merging 45a95f6 into 80bc6e4 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@p0mvn p0mvn changed the title feat: ristretto cache with memory bounding investigate: ristretto cache with memory bounding Jun 18, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 18, 2022

This pull request introduces 1 alert when merging a1247e1 into 80bc6e4 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@tac0turtle tac0turtle closed this Aug 3, 2022
@tac0turtle tac0turtle deleted the roman/ristretto-cache branch April 11, 2023 09:44
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