Skip to content

Commit 4c0c9b6

Browse files
authored
Avoid some uses of ConcurrentLinkedQueue.size() in SharedBlobCacheService (elastic#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.
1 parent 2d09714 commit 4c0c9b6

File tree

1 file changed

+8
-3
lines changed

1 file changed

+8
-3
lines changed

x-pack/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/shared/SharedBlobCacheService.java

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,7 @@ public boolean maybeFetchFullEntry(
500500
ActionListener<Void> listener
501501
) {
502502
int finalRegion = getEndingRegion(length);
503+
// TODO freeRegionCount uses freeRegions.size() which is is NOT a constant-time operation. Can we do better?
503504
if (freeRegionCount() < finalRegion) {
504505
// Not enough room to download a full file without evicting existing data, so abort
505506
listener.onResponse(null);
@@ -571,7 +572,7 @@ public void maybeFetchRegion(
571572
final Executor fetchExecutor,
572573
final ActionListener<Boolean> listener
573574
) {
574-
if (freeRegionCount() < 1 && maybeEvictLeastUsed() == false) {
575+
if (freeRegions.isEmpty() && maybeEvictLeastUsed() == false) {
575576
// no free page available and no old enough unused region to be evicted
576577
logger.info("No free regions, skipping loading region [{}]", region);
577578
listener.onResponse(false);
@@ -619,7 +620,7 @@ public void maybeFetchRange(
619620
final Executor fetchExecutor,
620621
final ActionListener<Boolean> listener
621622
) {
622-
if (freeRegionCount() < 1 && maybeEvictLeastUsed() == false) {
623+
if (freeRegions.isEmpty() && maybeEvictLeastUsed() == false) {
623624
// no free page available and no old enough unused region to be evicted
624625
logger.info("No free regions, skipping loading region [{}]", region);
625626
listener.onResponse(false);
@@ -671,7 +672,11 @@ private static void throwAlreadyClosed(String message) {
671672
throw new AlreadyClosedException(message);
672673
}
673674

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

0 commit comments

Comments
 (0)