Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bug fix: s3fifo with ghost #22149

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

Draft
cpegeric wants to merge 39 commits into matrixorigin:main
base: main
Choose a base branch
Loading
from cpegeric:s3fifo-reborn-refcnt

Conversation

Copy link
Contributor

@cpegeric cpegeric commented Jul 10, 2025
edited by qodo-merge-pro bot
Loading

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21838

What this PR does / why we need it:

  • The fix includes adding deleted status to avoid double postEvict/remove from hashtable.
  • Gemini AI also review and didn't detect any race condition
  • run test and benchmark with -race flag.
  • set the dellocated buffer to nil and panic when try to access deallocated buffer
  • defer Bytes.Release() at the end of postEvict() function
  • remove retain/release from post functions. Do retain/release inside the cache.

PR Type

Bug fix, Enhancement


Description

  • Fix S3-FIFO cache race conditions and memory management

  • Add proper reference counting for cache data

  • Implement thread-safe operations with mutex protection

  • Add ghost queue for S3-FIFO algorithm optimization


Changes diagram

flowchart LR
 A["Cache Operations"] --> B["Thread Safety"]
 B --> C["Reference Counting"]
 C --> D["Memory Management"]
 A --> E["S3-FIFO Algorithm"]
 E --> F["Ghost Queue"]
 F --> G["Eviction Policy"]
Loading

Changes walkthrough 📝

Relevant files
Configuration changes
4 files
config.go
Add disable S3-FIFO parameter to config
+1/-1
config.go
Add disable S3-FIFO parameter to config
+1/-1
cache.go
Add disable S3-FIFO configuration option
+1/-0
cache.go
Add disable S3-FIFO configuration support
+7/-5
Bug fix
6 files
bytes.go
Add mutex protection and panic checks
+44/-8
data_cache.go
Refactor with thread-safe operations
+14/-22
fifo.go
Complete S3-FIFO implementation with race fixes
+263/-192
queue.go
Add mutex protection to queue operations
+23/-4
io_vector.go
Update Release method with null checks
+7/-2
mem_cache.go
Remove manual retain/release calls
+3/-10
Tests
11 files
bytes_test.go
Add panic test cases for bytes
+19/-0
cache_test.go
Update test with disable S3-FIFO parameter
+1/-1
disk_cache_test.go
Update tests with disable S3-FIFO parameter
+14/-10
bench2_test.go
Add comprehensive cache benchmark tests
+133/-0
bench_test.go
Update benchmarks with disable S3-FIFO parameter
+6/-6
data_cache_test.go
Update tests and fix release method
+6/-2
fifo_test.go
Add comprehensive S3-FIFO algorithm tests
+124/-11
shardmap_test.go
Add shardmap functionality tests
+50/-0
local_fs_test.go
Update tests with disable S3-FIFO parameter
+2/-0
mem_cache_test.go
Update tests and add double free check
+9/-2
memory_fs_test.go
Update tests with disable S3-FIFO parameter
+2/-0
Enhancement
6 files
disk_cache.go
Add disable S3-FIFO parameter support
+2/-0
ghost.go
Implement ghost queue for S3-FIFO
+90/-0
shardmap.go
Implement thread-safe sharded hash map
+140/-0
data.go
Change Release method to return boolean
+1/-1
local_fs.go
Add disable S3-FIFO parameter support
+4/-1
s3_fs.go
Add disable S3-FIFO parameter support
+2/-0

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • cpegeric and others added 29 commits May 16, 2025 10:17
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @LeftHandCold LeftHandCold Awaiting requested review from LeftHandCold LeftHandCold is a code owner

    @XuPeng-SH XuPeng-SH Awaiting requested review from XuPeng-SH XuPeng-SH is a code owner

    @reusee reusee Awaiting requested review from reusee reusee is a code owner

    @fengttt fengttt Awaiting requested review from fengttt fengttt is a code owner

    @zhangxu19830126 zhangxu19830126 Awaiting requested review from zhangxu19830126 zhangxu19830126 is a code owner

    At least 1 approving review is required to merge this pull request.

    Assignees

    No one assigned

    Labels

    kind/bug Something isn't working Review effort 5/5 size/XL Denotes a PR that changes [1000, 1999] lines

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    AltStyle によって変換されたページ (->オリジナル) /