Add small changes for better Performance

* Cache addresses to avoid repeated lookups.
* Presize HashMap to avoid rehashing during record grouping
* Check queue size on flushes first and avoid creation of Duration objects
* Use ArrayList for better cache locality.

These changes were measure with the JMH benchmark to lead to significant
improvements. They were especially high on small record numbers with small
batches. But even on 5000 records cases the provided 30-60% high throughput.

Signed-off-by: Karsten Schnitter <[email protected]>

Author: KarstenSchnitter

data-prepper-core/src/jmh/java/org/opensearch/dataprepper/core/peerforwarder/RemotePeerForwarderBenchmark.java

@@ -52,7 +52,7 @@ public class RemotePeerForwarderBenchmark {
     private static final int BATCH_DELAY = 100;
     private static final int FAILED_FORWARDING_REQUEST_LOCAL_WRITE_TIMEOUT = 100;
     private static final int FORWARDING_BATCH_SIZE = BATCH_SIZE;
-    private static final int FORWARDING_BATCH_QUEUE_DEPTH = 3;
+    private static final int FORWARDING_BATCH_QUEUE_DEPTH = 25;
     private static final Duration FORWARDING_BATCH_TIMEOUT = Duration.ofMillis(800);
     private static final int PIPELINE_WORKER_THREADS = 8;
     private static final int HASH_RING_VIRTUAL_NODES = 128;

data-prepper-core/src/main/java/org/opensearch/dataprepper/core/peerforwarder/HashRing.java

@@ -38,6 +38,7 @@ public class HashRing implements Consumer<List<Endpoint>> {
     private final PeerListProvider peerListProvider;
 
     private TreeMap<BigInteger, String> hashServerMap = new TreeMap<>();
+    private int peerCount = 0;
 
     public HashRing(final PeerListProvider peerListProvider, final int numVirtualNodes) {
         Objects.requireNonNull(peerListProvider);
@@ -77,6 +78,10 @@ public Optional<String> getServerIp(final List<String> identificationKeyValues)
         }
     }
 
+    public int getPeerCount() {
+        return peerCount;
+    }
+
     @Override
     public void accept(final List<Endpoint> endpoints) {
         buildHashServerMap();
@@ -92,6 +97,7 @@ private void buildHashServerMap() {
         }
 
         this.hashServerMap = newHashValueMap;
+        this.peerCount = endpoints.size();
     }
 
     private void addServerIpToHashMap(final String serverIp, final Map<BigInteger, String> targetMap) {

data-prepper-core/src/main/java/org/opensearch/dataprepper/core/peerforwarder/RemotePeerForwarder.java

@@ -22,12 +22,10 @@
 import java.net.SocketException;
 import java.net.UnknownHostException;
 import java.time.Duration;
-import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -56,6 +54,7 @@ class RemotePeerForwarder implements PeerForwarder {
     private final Set<String> identificationKeys;
     final ConcurrentHashMap<String, LinkedBlockingQueue<Record<Event>>> peerBatchingQueueMap;
     private final ConcurrentHashMap<String, Long> peerBatchingLastFlushTimeMap;
+    private final ConcurrentHashMap<String, Boolean> localAddressCache;
 
     private final Counter recordsActuallyProcessedLocallyCounter;
     private final Counter recordsToBeProcessedLocallyCounter;
@@ -99,6 +98,7 @@ class RemotePeerForwarder implements PeerForwarder {
         this.pipelineWorkerThreads = pipelineWorkerThreads;
         peerBatchingQueueMap = new ConcurrentHashMap<>();
         peerBatchingLastFlushTimeMap = new ConcurrentHashMap<>();
+        localAddressCache = new ConcurrentHashMap<>();
 
         recordsActuallyProcessedLocallyCounter = pluginMetrics.counter(RECORDS_ACTUALLY_PROCESSED_LOCALLY);
         recordsToBeProcessedLocallyCounter = pluginMetrics.counter(RECORDS_TO_BE_PROCESSED_LOCALLY);
@@ -149,13 +149,13 @@ private Map<String, List<Record<Event>>> groupRecordsBasedOnIdentificationKeys(
             final Collection<Record<Event>> records,
             final Set<String> identificationKeys
     ) {
-        final Map<String, List<Record<Event>>> groupedRecords = new HashMap<>();
+        final Map<String, List<Record<Event>>> groupedRecords = new HashMap<>(hashRing.getPeerCount());
 
         // group records based on IP address calculated by HashRing
         for (final Record<Event> record : records) {
             final Event event = record.getData();
 
-            final List<String> identificationKeyValues = new LinkedList<>();
+            final List<String> identificationKeyValues = new ArrayList<>(identificationKeys.size());
             int numMissingIdentificationKeys = 0;
             for (final String identificationKey : identificationKeys) {
                 final Object identificationKeyValue = event.get(identificationKey, Object.class);
@@ -178,21 +178,23 @@ private Map<String, List<Record<Event>>> groupRecordsBasedOnIdentificationKeys(
     }
 
     private boolean isAddressDefinedLocally(final String address) {
-        final InetAddress inetAddress;
-        try {
-            inetAddress = InetAddress.getByName(address);
-        } catch (final UnknownHostException e) {
-            return false;
-        }
-        if (inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress()) {
-            return true;
-        } else {
+        return localAddressCache.computeIfAbsent(address, addr -> {
+            final InetAddress inetAddress;
             try {
-                return NetworkInterface.getByInetAddress(inetAddress) != null;
-            } catch (final SocketException e) {
+                inetAddress = InetAddress.getByName(addr);
+            } catch (final UnknownHostException e) {
                 return false;
             }
-        }
+            if (inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress()) {
+                return true;
+            } else {
+                try {
+                    return NetworkInterface.getByInetAddress(inetAddress) != null;
+                } catch (final SocketException e) {
+                    return false;
+                }
+            }
+        });
     }
 
     private List<Record<Event>> batchRecordsForForwarding(final String destinationIp, final List<Record<Event>> records) {
@@ -294,12 +296,14 @@ private List<Record<Event>> getRecordsToForward(final String destinationIp) {
     }
 
     private boolean shouldFlushBatch(final String destinationIp) {
-        final long currentTime = System.currentTimeMillis();
-        final long millisSinceLastFlush = currentTime - peerBatchingLastFlushTimeMap.getOrDefault(destinationIp, System.currentTimeMillis());
-        final Duration durationSinceLastFlush = Duration.of(millisSinceLastFlush, ChronoUnit.MILLIS);
+        final LinkedBlockingQueue<Record<Event>> queue = peerBatchingQueueMap.get(destinationIp);
+        if (queue.size() >= forwardingBatchSize) {
+            return true;
+        }
 
-        final boolean shouldFlushDueToTimeout = durationSinceLastFlush.compareTo(forwardingBatchTimeout) >= 0;
-        return shouldFlushDueToTimeout || peerBatchingQueueMap.get(destinationIp).size() >= forwardingBatchSize;
+        final long currentTime = System.currentTimeMillis();
+        final long millisSinceLastFlush = currentTime - peerBatchingLastFlushTimeMap.getOrDefault(destinationIp, currentTime);
+        return millisSinceLastFlush >= forwardingBatchTimeout.toMillis();
     }
 
     void processFailedRequestsLocally(final AggregatedHttpResponse httpResponse, final Collection<Record<Event>> records) {