Skip to content

Precompute the BitsetCacheKey hashCode, and account for the size of the key #132418

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

Closed
wants to merge 5 commits into from
Closed

Precompute the BitsetCacheKey hashCode, and account for the size of the key #132418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1b3a6f
Mark this class final, and whitespace
joegallo Aug 1, 2025
f60d917
Eagerly compute (and cache) the hashCode
joegallo Aug 1, 2025
02ac7a1
Account for the non-zero size of the key in the cache
joegallo Aug 1, 2025
5671066
Update docs/changelog/132418.yaml
joegallo Aug 4, 2025
6cefb2f
Merge branch 'main' into dls-cache-key-hashCode-and-size
joegallo Aug 5, 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
6 changes: 6 additions & 0 deletions docs/changelog/132418.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 132418
summary: "Precompute the `BitsetCacheKey` `hashCode,` and account for the size of\
\ the key"
area: Authorization
type: bug
issues: []
22 changes: 19 additions & 3 deletions .../org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -142,7 +143,7 @@ public DocumentSubsetBitsetCache(Settings settings, ThreadPool threadPool) {
this.bitsetCache = CacheBuilder.<BitsetCacheKey, BitSet>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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the intention but this concerns me a little. It means that the cache will now hold few items than it did before (for the same configuration). That's potentially going to mean someone's node ends up caching less than it used to, even though nothing has really changed.

Have we done any analysis on what it means for the overall cache size?

.removalListener(this::onCacheEviction)
.build();

Expand Down Expand Up @@ -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;"
Expand Down Expand Up @@ -325,12 +326,21 @@ public Map<String, Object> 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
Expand All @@ -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
Expand Down
29 changes: 17 additions & 12 deletions ...elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -126,15 +131,15 @@ 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));
}
});
}

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();
Expand All @@ -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");

Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down