Optimize some per-document hot paths in the geoip processor (#120824)

Author: joegallo

docs/changelog/120824.yaml

@@ -0,0 +1,5 @@
+pr: 120824
+summary: Optimize some per-document hot paths in the geoip processor
+area: Ingest Node
+type: enhancement
+issues: []

modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java

@@ -70,6 +70,7 @@
 import static org.elasticsearch.ingest.geoip.GeoIpTestUtils.copyDefaultDatabases;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.anEmptyMap;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -172,10 +173,15 @@ public void testInvalidTimestamp() throws Exception {
             for (Path geoIpTmpDir : geoIpTmpDirs) {
                 try (Stream<Path> files = Files.list(geoIpTmpDir)) {
                     Set<String> names = files.map(f -> f.getFileName().toString()).collect(Collectors.toSet());
-                    assertThat(names, not(hasItem("GeoLite2-ASN.mmdb")));
-                    assertThat(names, not(hasItem("GeoLite2-City.mmdb")));
-                    assertThat(names, not(hasItem("GeoLite2-Country.mmdb")));
-                    assertThat(names, not(hasItem("MyCustomGeoLite2-City.mmdb")));
+                    assertThat(
+                        names,
+                        allOf(
+                            not(hasItem("GeoLite2-ASN.mmdb")),
+                            not(hasItem("GeoLite2-City.mmdb")),
+                            not(hasItem("GeoLite2-Country.mmdb")),
+                            not(hasItem("MyCustomGeoLite2-City.mmdb"))
+                        )
+                    );
                 }
             }
         });

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

@@ -53,6 +53,10 @@ public class DatabaseReaderLazyLoader implements IpDatabase {
     private volatile boolean deleteDatabaseFileOnShutdown;
     private final AtomicInteger currentUsages = new AtomicInteger(0);
 
+    // it seems insane, especially if you read the code for UnixPath, but calling toString on a path in advance here is faster enough
+    // than calling it on every call to cache.putIfAbsent that it makes the slight additional internal complication worth it
+    private final String cachedDatabasePathToString;
+
     DatabaseReaderLazyLoader(GeoIpCache cache, Path databasePath, String md5) {
         this.cache = cache;
         this.databasePath = Objects.requireNonNull(databasePath);
@@ -61,6 +65,9 @@ public class DatabaseReaderLazyLoader implements IpDatabase {
         this.databaseReader = new SetOnce<>();
         this.databaseType = new SetOnce<>();
         this.buildDate = new SetOnce<>();
+
+        // cache the toString on construction
+        this.cachedDatabasePathToString = databasePath.toString();
     }
 
     /**
@@ -99,7 +106,7 @@ int current() {
     @Override
     @Nullable
     public <RESPONSE> RESPONSE getResponse(String ipAddress, CheckedBiFunction<Reader, String, RESPONSE, Exception> responseProvider) {
-        return cache.putIfAbsent(ipAddress, databasePath.toString(), ip -> {
+        return cache.putIfAbsent(ipAddress, cachedDatabasePathToString, ip -> {
             try {
                 return responseProvider.apply(get(), ipAddress);
             } catch (Exception e) {

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

@@ -206,12 +206,32 @@ public static Metadata fromXContent(XContentParser parser) {
         }
 
         public boolean isCloseToExpiration() {
-            return Instant.ofEpochMilli(lastCheck).isBefore(Instant.now().minus(25, ChronoUnit.DAYS));
+            final Instant now = Instant.ofEpochMilli(System.currentTimeMillis()); // millisecond precision is sufficient (and faster)
+            return Instant.ofEpochMilli(lastCheck).isBefore(now.minus(25, ChronoUnit.DAYS));
         }
 
+        // these constants support the micro optimization below, see that note
+        private static final TimeValue THIRTY_DAYS = TimeValue.timeValueDays(30);
+        private static final long THIRTY_DAYS_MILLIS = THIRTY_DAYS.millis();
+
         public boolean isNewEnough(Settings settings) {
-            TimeValue valid = settings.getAsTime("ingest.geoip.database_validity", TimeValue.timeValueDays(30));
-            return Instant.ofEpochMilli(lastCheck).isAfter(Instant.now().minus(valid.getMillis(), ChronoUnit.MILLIS));
+            // micro optimization: this looks a little silly, but the expected case is that database_validity is only used in tests.
+            // we run this code on every document, though, so the argument checking and other bits that getAsTime does is enough
+            // to show up in a flame graph.
+
+            // if you grep for "ingest.geoip.database_validity" and you'll see that it's not a 'real' setting -- it's only defined in
+            // AbstractGeoIpIT, that's why it's an inline string constant here and no some static final, and also why it cannot
+            // be the case that this setting exists in a real running cluster
+
+            final long valid;
+            if (settings.hasValue("ingest.geoip.database_validity")) {
+                valid = settings.getAsTime("ingest.geoip.database_validity", THIRTY_DAYS).millis();
+            } else {
+                valid = THIRTY_DAYS_MILLIS;
+            }
+
+            final Instant now = Instant.ofEpochMilli(System.currentTimeMillis()); // millisecond precision is sufficient (and faster)
+            return Instant.ofEpochMilli(lastCheck).isAfter(now.minus(valid, ChronoUnit.MILLIS));
         }
 
         @Override