From 6f303f5f532b42c0550ce00ed0ff7e484a8842eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 20 May 2025 11:35:06 +0200 Subject: [PATCH] Avoid some uses of ConcurrentLinkedQueue.size() in SharedBlobCacheService (#128119) SharedBlobCacheService keeps track of the free regions in a ConcurrentLinkedQueue. We use its "size()" method in three places outside of tests but unfortunately this is not a constant time operation because of the asynchronous nature of this queue. This change removes two of the uses where we only check if the queue is empty by calling the "isEmpty()" method instead. --- .../blobcache/shared/SharedBlobCacheService.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java b/x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java index ed7965b85a36a..05b39f32bfc7a 100644 --- a/x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java +++ b/x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java @@ -500,6 +500,7 @@ public boolean maybeFetchFullEntry( ActionListener 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); @@ -571,7 +572,7 @@ public void maybeFetchRegion( final Executor fetchExecutor, final ActionListener 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); @@ -619,7 +620,7 @@ public void maybeFetchRange( final Executor fetchExecutor, final ActionListener 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); @@ -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(); }