Skip to content

Avoid some uses of ConcurrentLinkedQueue.size() in SharedBlobCacheService #128119

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 20, 2025
Merged

Avoid some uses of ConcurrentLinkedQueue.size() in SharedBlobCacheService #128119

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f91c19
Avoid uses of ConcurrentLinkedQueue.size() in SharedBlobCacheService
cbuescher May 18, 2025
95a97f3
Update docs/changelog/128119.yaml
cbuescher May 19, 2025
2e0052a
Delete docs/changelog/128119.yaml
cbuescher May 19, 2025
2e5c0b0
Merge branch 'main' into improve-SharedBlobCacheService
cbuescher May 19, 2025
747b243
Add javadoc to freeRegionCount
cbuescher May 19, 2025
43bd7b7
Merge branch 'main' into improve-SharedBlobCacheService
cbuescher May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions ...n/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ public boolean maybeFetchFullEntry(
ActionListener<Void> listener
) {
int finalRegion = getEndingRegion(length);
// TODO freeRegionCount uses freeRegions.size() which is is NOT a constant-time operation. Can we do better?
if (freeRegionCount() < finalRegion) {
// Not enough room to download a full file without evicting existing data, so abort
listener.onResponse(null);
Expand Down Expand Up @@ -571,7 +572,7 @@ public void maybeFetchRegion(
final Executor fetchExecutor,
final ActionListener<Boolean> listener
) {
if (freeRegionCount() < 1 && maybeEvictLeastUsed() == false) {
if (freeRegions.isEmpty() && maybeEvictLeastUsed() == false) {
// no free page available and no old enough unused region to be evicted
logger.info("No free regions, skipping loading region [{}]", region);
listener.onResponse(false);
Expand Down Expand Up @@ -619,7 +620,7 @@ public void maybeFetchRange(
final Executor fetchExecutor,
final ActionListener<Boolean> listener
) {
if (freeRegionCount() < 1 && maybeEvictLeastUsed() == false) {
if (freeRegions.isEmpty() && maybeEvictLeastUsed() == false) {
// no free page available and no old enough unused region to be evicted
logger.info("No free regions, skipping loading region [{}]", region);
listener.onResponse(false);
Expand Down Expand Up @@ -671,7 +672,11 @@ private static void throwAlreadyClosed(String message) {
throw new AlreadyClosedException(message);
}

// used by tests
/**
* NOTE: Method is package private mostly to allow checking the number of fee regions in tests.
* However, it is also used by {@link SharedBlobCacheService#maybeFetchFullEntry} but we should try
* to move away from that because calling "size" on a ConcurrentLinkedQueue is not a constant time operation.
*/
int freeRegionCount() {
return freeRegions.size();
}
Expand Down