Optimize some per-document hot paths in the geoip processor

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