-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Reduce overhead in blob cache service get #96399
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
Reduce overhead in blob cache service get #96399
Conversation
Avoid the use of KeyedLock, which has a high overhead for uncontended locks. Reduce granularity of lock during #get to the actual region. Relates elastic#96372
Hi @henningandersen, I've created a changelog YAML for you. |
if (entry.chunk.sharedBytesPos == -1) { | ||
synchronized (entry.chunk) { | ||
if (entry.chunk.sharedBytesPos == -1) { | ||
if (keyMapping.get(regionKey) != entry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also assign -2 to sharedBytesPos
in this case instead. I think it does not really matter, we expect near no contention here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, hard to see how this wouldn't be a considerable win performance wise. Spent 20min thinking this through as well and couldn't find a safety issue with the change that wouldn't have been here already :)
Pinging @elastic/es-distributed (Team:Distributed) |
...ugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM :) Thanks Henning!
} | ||
if (Assertions.ENABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: maybe move this kind of assertion to a separate method (that can just include the next assertion below) to save a little on method size here? (probably not too relevant but also makes the method easier to read IMO)
Thanks Armin! |
In racy evict cases, assertions in blob cache did not hold, adapted test and added fixes. Relates elastic#96399
In racy evict cases, assertions in blob cache did not hold, adapted test and added fixes. Relates #96399
Avoid the use of KeyedLock, which has a high overhead for uncontended locks. Reduce granularity of lock during #get to the actual region.
Relates #96372