Do not call caching functions with mutex in bucketIndexReader #3400
Conversation
| @@ -413,9 +413,9 @@ func (r *bucketIndexReader) fetchPostings(ctx context.Context, keys []labels.Lab | |||
| // Return postings and fill LRU cache. | |||
| // Truncate first 4 bytes which are length of posting. | |||
| output[p.keyID] = newBigEndianPostings(pBytes[4:]) | |||
There was a problem hiding this comment.
Why do we need a lock around this access too? It is a slice and I assume we only assign each element once
There was a problem hiding this comment.
The answer is that we need to protect output from concurrency, so the lock is required. But we don"t need a per bucketIndexReader mutex: we can just have a local one. In the next PR I will get rid of bucketIndexReader.mtx completely.
There was a problem hiding this comment.
but isn"t output[p.keyID] = x executed only once for every possible p.keyID? My argument is that there is no concurrent access on the slice elements. Each goroutine accesses its own subset of output. Isn"t that true?
There was a problem hiding this comment.
You"re right. Unless bugs in the postings resizing, there"s no concurrent access for a given key and the slices is preallocated. I"m convinced we can remove it, but I still would prefer to do it in a separate PR (the one removing mtx at all) given this PR focus is "Do not call caching functions with mutex in bucketIndexReader".
pracucci
force-pushed
the
do-not-call-cache-functions-with-mutex
branch
from
8659f2a to
e6278c6
Compare
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
do-not-call-cache-functions-with-mutex
branch
from
e6278c6 to
bca260a
Compare
…a#3400) Signed-off-by: Marco Pracucci <[email protected]> Signed-off-by: Marco Pracucci <[email protected]>
What this PR does
In
bucketIndexReaderthere are a couple of places where we call the caching functions while holding thebucketIndexReadermutex. I think this is not required, as also pointed by Dimitar in this comment #3396 (comment).Why I think it"s not necessary:
bucketIndexReader.mtxis per tenant and block.bucketIndexReaderand I can"t see anything that requires holdingbucketIndexReader.mtxThis PR is on top of #3398.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]