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

fix: insert disk cache on hybrid cache fetch miss #591

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Conversation

MrCroxx
Copy link
Collaborator

@MrCroxx MrCroxx commented Jul 1, 2024

What's changed and what's your intention?

Please explain IN DETAIL what the changes are in this PR and why they are needed. :D

Previously, foyer inserts disk cache on in-memory cache eviction. After v0.9.x, foyer inserts disk cache on hybrid cache insertion. But fetch() only calls insert on in-memory cache, not hybrid cache insert().

This PR fixes the bug by proactively inserting the disk cache on hybrid fetch miss.

Benchmark (new baseline)

rm -rf /6536/foyer && cargo build --release && RUST_BACKTRACE=1 RUST_LOG=info ./target/release/foyer-bench --dir /6536/foyer --mem 64 --disk 102400 --file-size 64 --get-range 10000 --flushers 4 --reclaimers 4 --time 60 --writers 256 --w-rate 4 --admission-rate-limit 500 --readers 1024 --r-rate 1 --runtime --compression none --distribution zipf --distribution-zipf-s 0.8 --metrics
Total:
disk total iops: 3513.0
disk total throughput: 582.8 MiB/s
disk read iops: 1236.9
disk read throughput: 82.1 MiB/s
disk write iops: 2276.1
disk write throughput: 500.7 MiB/s
insert iops: 14290.2/s
insert throughput: 893.2 MiB/s
insert lat p50: 4us
insert lat p90: 7us
insert lat p99: 17us
insert lat p999: 40us
insert lat p9999: 140us
insert lat p99999: 365us
insert lat pmax: 1011us
get iops: 17296.4/s
get miss: 8.41% 
get throughput: 1.0 GiB/s
get hit lat p50: 3us
get hit lat p90: 1023us
get hit lat p99: 3071us
get hit lat p999: 4415us
get hit lat p9999: 5663us
get hit lat p99999: 6783us
get hit lat pmax: 8319us
get miss lat p50: 92us
get miss lat p90: 919us
get miss lat p99: 2623us
get miss lat p999: 4047us
get miss lat p9999: 4895us
get miss lat p99999: 6111us
get miss lat pmax: 6111us

Checklist

  • I have written the necessary rustdoc comments
  • I have added the necessary unit tests and integration tests
  • I have passed make all (or make fast instead if the old tests are not modified) in my local environment.

Related issues or PRs (optional)

risingwavelabs/risingwave#17473

@MrCroxx MrCroxx added the bug Something isn't working label Jul 1, 2024
@MrCroxx MrCroxx added this to the v0.9 milestone Jul 1, 2024
@MrCroxx MrCroxx self-assigned this Jul 1, 2024
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.10%. Comparing base (430e536) to head (96c5b37).

Files Patch % Lines
foyer/src/hybrid/cache.rs 85.18% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   86.14%   86.10%   -0.04%     
==========================================
  Files          59       59              
  Lines        7620     7631      +11     
==========================================
+ Hits         6564     6571       +7     
- Misses       1056     1060       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MrCroxx added a commit that referenced this pull request Jul 1, 2024
Copy link
Collaborator

@Li0k Li0k left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix

@MrCroxx MrCroxx merged commit 62ae5e7 into main Jul 1, 2024
16 checks passed
@MrCroxx MrCroxx deleted the xx/ffetch branch July 1, 2024 16:02
@MrCroxx MrCroxx modified the milestones: v0.9, v0.10 Jul 1, 2024
MrCroxx added a commit that referenced this pull request Jul 1, 2024
MrCroxx added a commit that referenced this pull request Jul 2, 2024
* refactor: remove atomic op in #591

Signed-off-by: MrCroxx <[email protected]>

* refactor: use deversion future to simplify? code

Signed-off-by: MrCroxx <[email protected]>

* chore: remove unused

Signed-off-by: MrCroxx <[email protected]>

* chore: pass typo

Signed-off-by: MrCroxx <[email protected]>

* chore: tiny refactor

Signed-off-by: MrCroxx <[email protected]>

* refactor: remove default bound to refine

Signed-off-by: MrCroxx <[email protected]>

* refactor: refine code

Signed-off-by: MrCroxx <[email protected]>

* chore: refine code

Signed-off-by: MrCroxx <[email protected]>

---------

Signed-off-by: MrCroxx <[email protected]>
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
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants