GeoIpCache and EnrichCache improvements

docs/changelog/132922.yaml

@@ -0,0 +1,5 @@
+pr: 132922
+summary: '`GeoIpCache` and `EnrichCache` improvements'
+area: Authorization
+type: enhancement
+issues: []

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java

@@ -17,7 +17,7 @@
 import org.elasticsearch.ingest.geoip.stats.CacheStats;
 
 import java.nio.file.Path;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.LongAdder;
 import java.util.function.Function;
 import java.util.function.LongSupplier;
 
@@ -44,8 +44,8 @@ public String toString() {
 
     private final LongSupplier relativeNanoTimeProvider;
     private final Cache<CacheKey, Object> cache;
-    private final AtomicLong hitsTimeInNanos = new AtomicLong(0);
-    private final AtomicLong missesTimeInNanos = new AtomicLong(0);
+    private final LongAdder hitsTimeInNanos = new LongAdder();
+    private final LongAdder missesTimeInNanos = new LongAdder();
 
     // package private for testing
     GeoIpCache(long maxSize, LongSupplier relativeNanoTimeProvider) {
@@ -62,27 +62,24 @@ public String toString() {
 
     @SuppressWarnings("unchecked")
     <RESPONSE> RESPONSE putIfAbsent(ProjectId projectId, String ip, String databasePath, Function<String, RESPONSE> retrieveFunction) {
+        long cacheStart = relativeNanoTimeProvider.getAsLong();
         // can't use cache.computeIfAbsent due to the elevated permissions for the jackson (run via the cache loader)
         CacheKey cacheKey = new CacheKey(projectId, ip, databasePath);
-        long cacheStart = relativeNanoTimeProvider.getAsLong();
         // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition.
         Object response = cache.get(cacheKey);
-        long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart;
 
         // populate the cache for this key, if necessary
         if (response == null) {
-            long retrieveStart = relativeNanoTimeProvider.getAsLong();
             response = retrieveFunction.apply(ip);
             // if the response from the database was null, then use the no-result sentinel value
             if (response == null) {
                 response = NO_RESULT;
             }
             // store the result or no-result in the cache
             cache.put(cacheKey, response);
-            long databaseRequestAndCachePutTime = relativeNanoTimeProvider.getAsLong() - retrieveStart;
-            missesTimeInNanos.addAndGet(cacheRequestTime + databaseRequestAndCachePutTime);
+            missesTimeInNanos.add(relativeNanoTimeProvider.getAsLong() - cacheStart);
         } else {
-            hitsTimeInNanos.addAndGet(cacheRequestTime);
+            hitsTimeInNanos.add(relativeNanoTimeProvider.getAsLong() - cacheStart);
         }
 
         if (response == NO_RESULT) {
@@ -126,8 +123,8 @@ public CacheStats getCacheStats() {
             stats.getHits(),
             stats.getMisses(),
             stats.getEvictions(),
-            TimeValue.nsecToMSec(hitsTimeInNanos.get()),
-            TimeValue.nsecToMSec(missesTimeInNanos.get())
+            TimeValue.nsecToMSec(hitsTimeInNanos.sum()),
+            TimeValue.nsecToMSec(missesTimeInNanos.sum())
         );
     }
 

modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java

@@ -133,8 +133,8 @@ public void testGetCacheStats() {
         assertThat(cacheStats.evictions(), equalTo(2L));
         // There are 3 hits, each taking 1ms:
         assertThat(cacheStats.hitsTimeInMillis(), equalTo(3L));
-        // There are 4 misses. Each is made up of a cache query, and a database query, each being 1ms:
-        assertThat(cacheStats.missesTimeInMillis(), equalTo(8L));
+        // There are 4 misses. each taking 1ms:
+        assertThat(cacheStats.missesTimeInMillis(), equalTo(4L));
     }
 
     public void testPurgeCacheEntriesForDatabase() {

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichCache.java

@@ -23,7 +23,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.LongAdder;
 import java.util.function.Consumer;
 import java.util.function.LongSupplier;
 import java.util.function.ToLongBiFunction;
@@ -47,9 +47,9 @@ public final class EnrichCache {
 
     private final Cache<CacheKey, CacheValue> cache;
     private final LongSupplier relativeNanoTimeProvider;
-    private final AtomicLong hitsTimeInNanos = new AtomicLong(0);
-    private final AtomicLong missesTimeInNanos = new AtomicLong(0);
-    private final AtomicLong sizeInBytes = new AtomicLong(0);
+    private final LongAdder hitsTimeInNanos = new LongAdder();
+    private final LongAdder missesTimeInNanos = new LongAdder();
+    private final LongAdder sizeInBytes = new LongAdder();
 
     EnrichCache(long maxSize) {
         this(maxSize, System::nanoTime);
@@ -72,7 +72,7 @@ public final class EnrichCache {
 
     private Cache<CacheKey, CacheValue> createCache(long maxWeight, ToLongBiFunction<CacheKey, CacheValue> weigher) {
         var builder = CacheBuilder.<CacheKey, CacheValue>builder().setMaximumWeight(maxWeight).removalListener(notification -> {
-            sizeInBytes.getAndAdd(-1 * notification.getValue().sizeInBytes);
+            sizeInBytes.add(-1 * notification.getValue().sizeInBytes);
         });
         if (weigher != null) {
             builder.weigher(weigher);
@@ -103,18 +103,15 @@ public void computeIfAbsent(
         long cacheStart = relativeNanoTimeProvider.getAsLong();
         var cacheKey = new CacheKey(projectId, enrichIndex, lookupValue, maxMatches);
         List<Map<?, ?>> response = get(cacheKey);
-        long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart;
         if (response != null) {
-            hitsTimeInNanos.addAndGet(cacheRequestTime);
+            hitsTimeInNanos.add(relativeNanoTimeProvider.getAsLong() - cacheStart);
             listener.onResponse(response);
         } else {
-            final long retrieveStart = relativeNanoTimeProvider.getAsLong();
             searchResponseFetcher.accept(ActionListener.wrap(resp -> {
                 CacheValue cacheValue = toCacheValue(resp);
                 put(cacheKey, cacheValue);
                 List<Map<?, ?>> copy = deepCopy(cacheValue.hits, false);
-                long databaseQueryAndCachePutTime = relativeNanoTimeProvider.getAsLong() - retrieveStart;
-                missesTimeInNanos.addAndGet(cacheRequestTime + databaseQueryAndCachePutTime);
+                missesTimeInNanos.add(relativeNanoTimeProvider.getAsLong() - cacheStart);
                 listener.onResponse(copy);
             }, listener::onFailure));
         }
@@ -133,7 +130,7 @@ public void computeIfAbsent(
     // non-private for unit testing only
     void put(CacheKey cacheKey, CacheValue cacheValue) {
         cache.put(cacheKey, cacheValue);
-        sizeInBytes.addAndGet(cacheValue.sizeInBytes);
+        sizeInBytes.add(cacheValue.sizeInBytes);
     }
 
     public EnrichStatsAction.Response.CacheStats getStats(String localNodeId) {
@@ -144,9 +141,9 @@ public EnrichStatsAction.Response.CacheStats getStats(String localNodeId) {
             cacheStats.getHits(),
             cacheStats.getMisses(),
             cacheStats.getEvictions(),
-            TimeValue.nsecToMSec(hitsTimeInNanos.get()),
-            TimeValue.nsecToMSec(missesTimeInNanos.get()),
-            sizeInBytes.get()
+            TimeValue.nsecToMSec(hitsTimeInNanos.sum()),
+            TimeValue.nsecToMSec(missesTimeInNanos.sum()),
+            sizeInBytes.sum()
         );
     }
 

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichCacheTests.java

@@ -140,7 +140,7 @@ public void testComputeIfAbsent() throws InterruptedException {
             assertThat(cacheStats.misses(), equalTo(++expectedMisses));
             assertThat(cacheStats.evictions(), equalTo(0L));
             assertThat(cacheStats.hitsTimeInMillis(), equalTo(0L));
-            assertThat(cacheStats.missesTimeInMillis(), equalTo(2L)); // cache query and enrich query + cache put
+            assertThat(cacheStats.missesTimeInMillis(), equalTo(1L));
         }
 
         {
@@ -156,7 +156,7 @@ public void testComputeIfAbsent() throws InterruptedException {
             assertThat(cacheStats.misses(), equalTo(expectedMisses));
             assertThat(cacheStats.evictions(), equalTo(0L));
             assertThat(cacheStats.hitsTimeInMillis(), equalTo(1L));
-            assertThat(cacheStats.missesTimeInMillis(), equalTo(2L));
+            assertThat(cacheStats.missesTimeInMillis(), equalTo(1L));
         }
 
         {