benwtrent commented on code in PR #13124:
URL: https://github.com/apache/lucene/pull/13124#discussion_r1513208670
##########
lucene/core/src/java/org/apache/lucene/index/ConcurrentMergeScheduler.java:
##########
@@ -910,4 +936,68 @@ public void setSuppressExceptions(ConcurrentMergeScheduler
cms) {
}
});
}
+
+ private class ScaledExecutor implements Executor {
+
+ private final AtomicInteger activeCount = new AtomicInteger(0);
+ private final ThreadPoolExecutor executor;
+
+ public ScaledExecutor() {
+ this.executor =
+ new ThreadPoolExecutor(
+ 0, Math.max(1, maxThreadCount), 1L, TimeUnit.MINUTES, new
SynchronousQueue<>());
+ }
+
+ void shutdown() {
+ executor.shutdown();
+ }
+
+ private void updatePoolSize() {
+ executor.setMaximumPoolSize(Math.max(1, maxThreadCount));
+ }
+
+ @Override
+ public void execute(Runnable command) {
+ assert mergeThreads.contains(Thread.currentThread()) : "caller is not a
merge thread";
+ Thread currentThread = Thread.currentThread();
+ if (currentThread instanceof MergeThread mergeThread) {
+ execute(mergeThread, command);
+ } else {
+ command.run();
+ }
+ }
+
+ private void execute(MergeThread mergeThread, Runnable command) {
+ // don't do multithreaded merges for small merges
+ if (mergeThread.merge.estimatedMergeBytes < MIN_BIG_MERGE_MB * 1024 *
1024) {
+ command.run();
+ } else {
+ final boolean isThreadAvailable;
+ // we need to check if a thread is available before submitting the
task to the executor
+ // synchronize on CMS to get an accurate count of current threads
+ synchronized (ConcurrentMergeScheduler.this) {
+ int max = maxThreadCount - mergeThreads.size();
+ int value = activeCount.get();
+ if (value < max) {
+ activeCount.incrementAndGet();
+ isThreadAvailable = true;
+ } else {
+ isThreadAvailable = false;
+ }
+ }
Review Comment:
One major decision I made here is that CMS thread throttling knows nothing
about intra-merge threads.
This means, if there are intra-merge threads running, it is possible that
concurrency creeps above `maxThreadCount` until the intra-merge thread(s)
finish(es).
Making throttling & overall CMS behavior more aware of intra-merge thread
usage will be a much trickier change. I wasn't sure it was worth it. I am open
to opinions on this @msokolov @jpountz .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
