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

Handle usize overflow on big cache capacity #28

Merged
merged 7 commits into from
Aug 29, 2021

Conversation

tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 commented Aug 28, 2021

This PR fixes the issue #25, usize multiply overflow on big cache capacity.

Changes

  • Removed the multiply operation (max_capacity * 32) when setting the capacity of the popularity estimator FrequencySketch.
    • The implementation of the estimator was replaced in an older version of Moka and the multiply operation was not really needed since then.
  • Added code to ensure that the capacity of the popularity estimator always fits within a range of 128u32..=u32::MAX.
  • Changed the type of the internal fields of the estimator from usize to u32 so that the estimator will work consistency on any platform with different pointer width.

Verification

  • Added unit tests to sync::base_cache and unsync::cache modules to ensure overflows should not happen and sizes of the population estimators are correct:
    • Before applying the fix, ran the tests with the debug profile and confirmed they panicked as expected:
      ---- sync::base_cache::tests::test_skt_capacity_will_not_overflow stdout ----
      thread 'sync::base_cache::tests::test_skt_capacity_will_not_overflow' panicked at
      'attempt to multiply with overflow', src/sync/base_cache.rs:395:39
      
    • After applying the fix, ran the tests again and verified they passed. PASSED
  • Ran some mokabench workloads on Moka before/after the fix and verified that hit rates and run-time durations did not change. PASSED

Fixes #25

- Remove the multiply operation (`max_capacity * 32`) when setting the
  capacity of the popularity estimator (`FrequencySketch`).
    - The implementation of the estimator was replaced in an older version
      of Moka and the multiply operation was not really needed since then.
- Add code to ensure that the capacity of the popularity estimator always
  fits within a range of `128u32..=u32::MAX`.
- Change the type of the internal fields of the estimator from `usize` to
  `u32` so that the estimator will work consistency on any platform with
  different pointer width.
@tatsuya6502 tatsuya6502 linked an issue Aug 28, 2021 that may be closed by this pull request
- Reduce the max size of the popularity estimator on platforms with
  32-bit addressing.
- Change the type of the internal table of the estimator from `Vec<u64>`
  to `Box<[u64]>`.
@tatsuya6502 tatsuya6502 self-assigned this Aug 28, 2021
@tatsuya6502 tatsuya6502 added this to the v0.5.2 milestone Aug 28, 2021
- Add unit test to ensure overflows not to happen and the sizes of
  the population estimators are correct.
@tatsuya6502 tatsuya6502 marked this pull request as ready for review August 29, 2021 11:31
- Not a bug but for clarity, avoid a lossy cast from `u64` to `u32` before
  applying the table mask.
Copy link
Member Author

@tatsuya6502 tatsuya6502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging.

@tatsuya6502 tatsuya6502 merged commit 0ad38cc into maint-0.5 Aug 29, 2021
@tatsuya6502 tatsuya6502 deleted the fix-usize-overflow branch August 29, 2021 12:09
@tatsuya6502 tatsuya6502 added the bug Something isn't working label Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] usize overflow on big cache capacity
1 participant