From 830599547d4c65dd17d82dcbcc4045abbb19caa1 Mon Sep 17 00:00:00 2001
From: Luca Cavanna <javanna@apache.org>
Date: Wed, 10 May 2023 23:53:24 +0200
Subject: [PATCH] Simplify SliceExecutor and QueueSizeBasedExecutor

The only behaviour that QueueSizeBasedExecutor overrides from SliceExecutor is when to execute on the caller thread. There is no need to override the whole invokeAll method for that. Instead, this commit introduces a shouldExecuteOnCallerThread method that can be overridden.
---
 .../lucene/search/QueueSizeBasedExecutor.java | 27 ++-------
 .../apache/lucene/search/SliceExecutor.java   | 56 ++++++-------------
 .../lucene/search/TestIndexSearcher.java      | 13 ++---
 3 files changed, 25 insertions(+), 71 deletions(-)

diff --git a/lucene/core/src/java/org/apache/lucene/search/QueueSizeBasedExecutor.java b/lucene/core/src/java/org/apache/lucene/search/QueueSizeBasedExecutor.java
index a76b81f5da19..65ba1ea5573d 100644
--- a/lucene/core/src/java/org/apache/lucene/search/QueueSizeBasedExecutor.java
+++ b/lucene/core/src/java/org/apache/lucene/search/QueueSizeBasedExecutor.java
@@ -17,7 +17,6 @@
 
 package org.apache.lucene.search;
 
-import java.util.Collection;
 import java.util.concurrent.ThreadPoolExecutor;
 
 /**
@@ -30,31 +29,15 @@ class QueueSizeBasedExecutor extends SliceExecutor {
 
   private final ThreadPoolExecutor threadPoolExecutor;
 
-  public QueueSizeBasedExecutor(ThreadPoolExecutor threadPoolExecutor) {
+  QueueSizeBasedExecutor(ThreadPoolExecutor threadPoolExecutor) {
     super(threadPoolExecutor);
     this.threadPoolExecutor = threadPoolExecutor;
   }
 
   @Override
-  public void invokeAll(Collection<? extends Runnable> tasks) {
-    int i = 0;
-
-    for (Runnable task : tasks) {
-      boolean shouldExecuteOnCallerThread = false;
-
-      // Execute last task on caller thread
-      if (i == tasks.size() - 1) {
-        shouldExecuteOnCallerThread = true;
-      }
-
-      if (threadPoolExecutor.getQueue().size()
-          >= (threadPoolExecutor.getMaximumPoolSize() * LIMITING_FACTOR)) {
-        shouldExecuteOnCallerThread = true;
-      }
-
-      processTask(task, shouldExecuteOnCallerThread);
-
-      ++i;
-    }
+  boolean shouldExecuteOnCallerThread(int index, int numTasks) {
+    return super.shouldExecuteOnCallerThread(index, numTasks)
+        || threadPoolExecutor.getQueue().size()
+            >= (threadPoolExecutor.getMaximumPoolSize() * LIMITING_FACTOR);
   }
 }
diff --git a/lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java b/lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java
index c84beeb5fb78..a3d3325e7fd9 100644
--- a/lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java
+++ b/lucene/core/src/java/org/apache/lucene/search/SliceExecutor.java
@@ -18,6 +18,7 @@
 package org.apache.lucene.search;
 
 import java.util.Collection;
+import java.util.Objects;
 import java.util.concurrent.Executor;
 import java.util.concurrent.RejectedExecutionException;
 
@@ -28,54 +29,29 @@
 class SliceExecutor {
   private final Executor executor;
 
-  public SliceExecutor(Executor executor) {
-    this.executor = executor;
+  SliceExecutor(Executor executor) {
+    this.executor = Objects.requireNonNull(executor, "Executor is null");
   }
 
-  public void invokeAll(Collection<? extends Runnable> tasks) {
-
-    if (tasks == null) {
-      throw new IllegalArgumentException("Tasks is null");
-    }
-
-    if (executor == null) {
-      throw new IllegalArgumentException("Executor is null");
-    }
-
+  final void invokeAll(Collection<? extends Runnable> tasks) {
     int i = 0;
-
     for (Runnable task : tasks) {
-      boolean shouldExecuteOnCallerThread = false;
-
-      // Execute last task on caller thread
-      if (i == tasks.size() - 1) {
-        shouldExecuteOnCallerThread = true;
+      if (shouldExecuteOnCallerThread(i, tasks.size())) {
+        task.run();
+      } else {
+        try {
+          executor.execute(task);
+        } catch (
+            @SuppressWarnings("unused")
+            RejectedExecutionException e) {
+          task.run();
+        }
       }
-
-      processTask(task, shouldExecuteOnCallerThread);
       ++i;
     }
-    ;
   }
 
-  // Helper method to execute a single task
-  protected void processTask(final Runnable task, final boolean shouldExecuteOnCallerThread) {
-    if (task == null) {
-      throw new IllegalArgumentException("Input is null");
-    }
-
-    if (!shouldExecuteOnCallerThread) {
-      try {
-        executor.execute(task);
-
-        return;
-      } catch (
-          @SuppressWarnings("unused")
-          RejectedExecutionException e) {
-        // Execute on caller thread
-      }
-    }
-
-    task.run();
+  boolean shouldExecuteOnCallerThread(int index, int numTasks) {
+    return index == numTasks - 1;
   }
 }
diff --git a/lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java b/lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java
index c3bcda9d47a6..7e8693bd8b31 100644
--- a/lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java
+++ b/lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java
@@ -453,20 +453,15 @@ private void runSliceExecutorTest(ThreadPoolExecutor service, boolean useRandomS
     }
   }
 
-  private class RandomBlockingSliceExecutor extends SliceExecutor {
+  private static class RandomBlockingSliceExecutor extends SliceExecutor {
 
-    public RandomBlockingSliceExecutor(Executor executor) {
+    RandomBlockingSliceExecutor(Executor executor) {
       super(executor);
     }
 
     @Override
-    public void invokeAll(Collection<? extends Runnable> tasks) {
-
-      for (Runnable task : tasks) {
-        boolean shouldExecuteOnCallerThread = random().nextBoolean();
-
-        processTask(task, shouldExecuteOnCallerThread);
-      }
+    boolean shouldExecuteOnCallerThread(int index, int numTasks) {
+      return random().nextBoolean();
     }
   }
 }