diff --git a/docs/changelog/132418.yaml b/docs/changelog/132418.yaml new file mode 100644 index 0000000000000..58f1086d87caa --- /dev/null +++ b/docs/changelog/132418.yaml @@ -0,0 +1,6 @@ +pr: 132418 +summary: "Precompute the `BitsetCacheKey` `hashCode,` and account for the size of\ + \ the key" +area: Authorization +type: bug +issues: [] 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..b99a80f273ff5 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 @@ -23,6 +23,7 @@ import org.apache.lucene.util.Accountable; import org.apache.lucene.util.BitSet; import org.apache.lucene.util.FixedBitSet; +import org.apache.lucene.util.RamUsageEstimator; import org.elasticsearch.common.cache.Cache; import org.elasticsearch.common.cache.CacheBuilder; import org.elasticsearch.common.cache.RemovalNotification; @@ -142,7 +143,7 @@ public DocumentSubsetBitsetCache(Settings settings, ThreadPool threadPool) { this.bitsetCache = CacheBuilder.builder() .setExpireAfterAccess(ttl) .setMaximumWeight(maxWeightBytes) - .weigher((key, bitSet) -> bitSet == NULL_MARKER ? 0 : bitSet.ramBytesUsed()) + .weigher((key, bitSet) -> BitsetCacheKey.SHALLOW_SIZE + (bitSet == NULL_MARKER ? 0 : bitSet.ramBytesUsed())) .removalListener(this::onCacheEviction) .build(); @@ -247,7 +248,7 @@ public BitSet getBitSet(final Query query, final LeafReaderContext context) thro // A cache loader is not allowed to return null, return a marker object instead. return NULL_MARKER; } - final long bitSetBytes = result.ramBytesUsed(); + final long bitSetBytes = result.ramBytesUsed(); // note: this ignores the size of the key itself, but that's okay if (bitSetBytes > this.maxWeightBytes) { logger.warn( "built a DLS BitSet that uses [{}] bytes; the DLS BitSet cache has a maximum size of [{}] bytes;" @@ -325,12 +326,21 @@ public Map usageStats() { private static final class BitsetCacheKey { + // the shallow size accounts for the object itself, but it ignores the index and the query. + // the index is owned by the associated IndexReader and the query is shared, so they don't count against us here. + private static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(BitsetCacheKey.class); + 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 +355,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 diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java index 0645ea8b43b16..4dd6d188d78fa 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java @@ -76,9 +76,14 @@ public class DocumentSubsetBitsetCacheTests extends ESTestCase { private static final int FIELD_COUNT = 10; + // This comes from the SHALLOW_SIZE of the BitsetCacheKey + private static final long EXPECTED_BYTES_PER_BIT_SET_KEY = 24; /* key size */ // This value is based on the internal implementation details of lucene's FixedBitSet // If the implementation changes, this can be safely updated to match the new ram usage for a single bitset - private static final long EXPECTED_BYTES_PER_BIT_SET = 56; + private static final long EXPECTED_BYTES_PER_BIT_SET = 56; /* non-null value size */ + // a non-null entry will have the size of the key and the bit set, while a null entry would just be the size of the key + private static final long EXPECTED_BYTES_PER_ENTRY = EXPECTED_BYTES_PER_BIT_SET_KEY + EXPECTED_BYTES_PER_BIT_SET; + private ExecutorService singleThreadExecutor; @Before @@ -126,7 +131,7 @@ public void testNullEntriesAreNotCountedInMemoryUsage() throws Exception { final Query query = QueryBuilders.termQuery("dne-" + i, "dne- " + i).toQuery(searchExecutionContext); final BitSet bitSet = cache.getBitSet(query, leafContext); assertThat(bitSet, nullValue()); - assertThat(cache.ramBytesUsed(), equalTo(0L)); + assertThat(cache.ramBytesUsed(), equalTo(EXPECTED_BYTES_PER_BIT_SET_KEY * i)); assertThat(cache.entryCount(), equalTo(i)); } }); @@ -134,7 +139,7 @@ public void testNullEntriesAreNotCountedInMemoryUsage() throws Exception { public void testCacheRespectsMemoryLimit() throws Exception { // Enough to hold exactly 2 bit-sets in the cache - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET * 2; + final long maxCacheBytes = EXPECTED_BYTES_PER_ENTRY * 2; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); @@ -155,24 +160,24 @@ public void testCacheRespectsMemoryLimit() throws Exception { // The first time through we have 1 entry, after that we have 2 final int expectedCount = i == 1 ? 1 : 2; assertThat(cache.entryCount(), equalTo(expectedCount)); - assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_BIT_SET)); + assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_ENTRY)); // Older queries should get evicted, but the query from last iteration should still be cached if (previousQuery != null) { assertThat(cache.getBitSet(previousQuery, leafContext), sameInstance(previousBitSet)); assertThat(cache.entryCount(), equalTo(expectedCount)); - assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_BIT_SET)); + assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_ENTRY)); } previousQuery = query; previousBitSet = bitSet; assertThat(cache.getBitSet(queryBuilder.toQuery(searchExecutionContext), leafContext), sameInstance(bitSet)); assertThat(cache.entryCount(), equalTo(expectedCount)); - assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_BIT_SET)); + assertThat(cache.ramBytesUsed(), equalTo(expectedCount * EXPECTED_BYTES_PER_ENTRY)); } assertThat(cache.entryCount(), equalTo(2)); - assertThat(cache.ramBytesUsed(), equalTo(2 * EXPECTED_BYTES_PER_BIT_SET)); + assertThat(cache.ramBytesUsed(), equalTo(2 * EXPECTED_BYTES_PER_ENTRY)); cache.clear("testing"); @@ -183,7 +188,7 @@ public void testCacheRespectsMemoryLimit() throws Exception { public void testLogWarningIfBitSetExceedsCacheSize() throws Exception { // Enough to hold less than 1 bit-sets in the cache - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET - EXPECTED_BYTES_PER_BIT_SET / 3; + final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET - 1; // n.b. we're ignoring the key size, it's okay final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); @@ -220,7 +225,7 @@ public void testLogWarningIfBitSetExceedsCacheSize() throws Exception { public void testLogMessageIfCacheFull() throws Exception { // Enough to hold slightly more than 1 bit-sets in the cache - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET + EXPECTED_BYTES_PER_BIT_SET / 3; + final long maxCacheBytes = EXPECTED_BYTES_PER_ENTRY + EXPECTED_BYTES_PER_ENTRY / 3; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); @@ -282,7 +287,7 @@ public void testCacheRespectsAccessTimeExpiry() throws Exception { public void testIndexLookupIsClearedWhenBitSetIsEvicted() throws Exception { // Enough to hold slightly more than 1 bit-set in the cache - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET + EXPECTED_BYTES_PER_BIT_SET / 2; + final long maxCacheBytes = EXPECTED_BYTES_PER_ENTRY + EXPECTED_BYTES_PER_ENTRY / 2; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); @@ -331,7 +336,7 @@ public void testCacheUnderConcurrentAccess() throws Exception { // Force cache evictions by setting the size to be less than the number of distinct queries we search on. final int maxCacheCount = randomIntBetween(FIELD_COUNT / 2, FIELD_COUNT * 3 / 4); - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET * maxCacheCount; + final long maxCacheBytes = EXPECTED_BYTES_PER_ENTRY * maxCacheCount; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build(); @@ -410,7 +415,7 @@ public void testCacheUnderConcurrentAccess() throws Exception { public void testCleanupWorksWhenIndexIsClosing() throws Exception { // Enough to hold slightly more than 1 bit-set in the cache - final long maxCacheBytes = EXPECTED_BYTES_PER_BIT_SET + EXPECTED_BYTES_PER_BIT_SET / 2; + final long maxCacheBytes = EXPECTED_BYTES_PER_ENTRY + EXPECTED_BYTES_PER_ENTRY / 2; final Settings settings = Settings.builder() .put(DocumentSubsetBitsetCache.CACHE_SIZE_SETTING.getKey(), maxCacheBytes + "b") .build();