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

cache: readShard: use a separate mutex #4274

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RaduBerinde
Copy link
Member

We use a separate readShard mutex to to protect the readMap and
each entry's ref count. This removes the possibility of a couple of
cornercases, making the code much cleaner. We also now avoid
write-locking the entire cache shard when we start a read.

This will also potentially allow moving read buffer management into
readShard in the future (and having separate readShard sets per
store).

We use a separate `readShard` mutex to to protect the `readMap` and
each entry's ref count. This removes the possibility of a couple of
cornercases, making the code much cleaner. We also now avoid
write-locking the entire cache shard when we start a read.

This will also potentially allow moving read buffer management into
`readShard` in the future (and having separate `readShard` sets per
store).
@RaduBerinde RaduBerinde requested a review from a team as a code owner January 21, 2025 20:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaduBerinde)


internal/cache/refcnt_tracing.go line 54 at r1 (raw file):

}

func (v *refcnt) value() int32 {

this isn't needed either, yes?


internal/cache/read_shard.go line 51 at r1 (raw file):

//     creating a readEntry that is no longer needed because someone else has
//     done the read since the miss and inserted into the cache. By making the
//     readShard use shard.mu, such a race is avoided. A side benefit is that

This comment is a bit stale.


internal/cache/read_shard.go line 59 at r1 (raw file):

//     is the possibility that the compaction reader and a user-facing iterator
//     reader will do duplicate reads, but we accept that deficiency.
//     TODO(radu): consider using a separate mutex for the readShard; we are

Isn't this done by this PR?

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.

3 participants