From 2baa7f1b44e89013859c38156314ea0fdefae1ac Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 1 Aug 2025 16:10:25 -0400 Subject: [PATCH 1/2] Eagerly compute (and cache) the hashCode This actually matters, because while the hashCode for a BooleanQuery is already cached, it's not eagerly computed. Pre-computing the hashCode moves the initial calculation of the hashCode outside of anywhere that we're holding a lock. --- .../accesscontrol/DocumentSubsetBitsetCache.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java index 18c13860efd6a..6c25cb8d2fa7c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java @@ -327,10 +327,15 @@ private static final class BitsetCacheKey { final IndexReader.CacheKey index; final Query query; + final int hashCode; private BitsetCacheKey(IndexReader.CacheKey index, Query query) { this.index = index; this.query = query; + // compute the hashCode eagerly, since it's used multiple times in the cache implementation anyway -- the query here will + // be a ConstantScoreQuery around a BooleanQuery, and BooleanQuery already *lazily* caches the hashCode, so this isn't + // altogether that much faster in reality, but it makes it more explicit here that we're doing this + this.hashCode = computeHashCode(); } @Override @@ -345,9 +350,15 @@ public boolean equals(Object other) { return Objects.equals(this.index, that.index) && Objects.equals(this.query, that.query); } + private int computeHashCode() { + int result = index.hashCode(); + result = 31 * result + query.hashCode(); + return result; + } + @Override public int hashCode() { - return Objects.hash(index, query); + return hashCode; } @Override From 1dbf57a576e6a769c2dc56eddbcbc1352d10f9be Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Aug 2025 15:25:13 -0400 Subject: [PATCH 2/2] Apply some naming consistency by always calling the IndexReader.CacheKey 'indexKey' and always calling the BitsetCacheKey 'cacheKey'. --- .../DocumentSubsetBitsetCache.java | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java index 6c25cb8d2fa7c..8bd64086a5c4e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java @@ -151,8 +151,8 @@ public DocumentSubsetBitsetCache(Settings settings, ThreadPool threadPool) { } @Override - public void onClose(IndexReader.CacheKey ownerCoreCacheKey) { - final Set keys = keysByIndex.remove(ownerCoreCacheKey); + public void onClose(IndexReader.CacheKey indexKey) { + final Set keys = keysByIndex.remove(indexKey); if (keys != null) { // Because this Set has been removed from the map, and the only update to the set is performed in a // Map#compute call, it should not be possible to get a concurrent modification here. @@ -164,10 +164,10 @@ public void onClose(IndexReader.CacheKey ownerCoreCacheKey) { * Cleanup (synchronize) the internal state when an object is removed from the primary cache */ private void onCacheEviction(RemovalNotification notification) { - final BitsetCacheKey bitsetKey = notification.getKey(); - final IndexReader.CacheKey indexKey = bitsetKey.index; - if (keysByIndex.getOrDefault(indexKey, Set.of()).contains(bitsetKey) == false) { - // If the bitsetKey isn't in the lookup map, then there's nothing to synchronize + final BitsetCacheKey cacheKey = notification.getKey(); + final IndexReader.CacheKey indexKey = cacheKey.indexKey; + if (keysByIndex.getOrDefault(indexKey, Set.of()).contains(cacheKey) == false) { + // If the cacheKey isn't in the lookup map, then there's nothing to synchronize return; } // We push this to a background thread, so that it reduces the risk of blocking searches, but also so that the lock management is @@ -177,9 +177,9 @@ private void onCacheEviction(RemovalNotification notific cleanupExecutor.submit(() -> { try (ReleasableLock ignored = cacheEvictionLock.acquire()) { // it's possible for the key to be back in the cache if it was immediately repopulated after it was evicted, so check - if (bitsetCache.get(bitsetKey) == null) { + if (bitsetCache.get(cacheKey) == null) { // key is no longer in the cache, make sure it is no longer in the lookup map either. - Optional.ofNullable(keysByIndex.get(indexKey)).ifPresent(set -> set.remove(bitsetKey)); + Optional.ofNullable(keysByIndex.get(indexKey)).ifPresent(set -> set.remove(cacheKey)); } } }); @@ -325,12 +325,12 @@ public Map usageStats() { private static final class BitsetCacheKey { - final IndexReader.CacheKey index; + final IndexReader.CacheKey indexKey; final Query query; final int hashCode; - private BitsetCacheKey(IndexReader.CacheKey index, Query query) { - this.index = index; + private BitsetCacheKey(IndexReader.CacheKey indexKey, Query query) { + this.indexKey = indexKey; this.query = query; // compute the hashCode eagerly, since it's used multiple times in the cache implementation anyway -- the query here will // be a ConstantScoreQuery around a BooleanQuery, and BooleanQuery already *lazily* caches the hashCode, so this isn't @@ -347,11 +347,11 @@ public boolean equals(Object other) { return false; } final BitsetCacheKey that = (BitsetCacheKey) other; - return Objects.equals(this.index, that.index) && Objects.equals(this.query, that.query); + return Objects.equals(this.indexKey, that.indexKey) && Objects.equals(this.query, that.query); } private int computeHashCode() { - int result = index.hashCode(); + int result = indexKey.hashCode(); result = 31 * result + query.hashCode(); return result; } @@ -363,7 +363,7 @@ public int hashCode() { @Override public String toString() { - return getClass().getSimpleName() + "(" + index + "," + query + ")"; + return getClass().getSimpleName() + "(" + indexKey + "," + query + ")"; } } @@ -373,15 +373,15 @@ public String toString() { */ void verifyInternalConsistency() { this.bitsetCache.keys().forEach(bck -> { - final Set set = this.keysByIndex.get(bck.index); + final Set set = this.keysByIndex.get(bck.indexKey); if (set == null) { throw new IllegalStateException( - "Key [" + bck + "] is in the cache, but there is no entry for [" + bck.index + "] in the lookup map" + "Key [" + bck + "] is in the cache, but there is no entry for [" + bck.indexKey + "] in the lookup map" ); } if (set.contains(bck) == false) { throw new IllegalStateException( - "Key [" + bck + "] is in the cache, but the lookup entry for [" + bck.index + "] does not contain that key" + "Key [" + bck + "] is in the cache, but the lookup entry for [" + bck.indexKey + "] does not contain that key" ); } });