From b22400454ddaccb526c08cbb561777312d219c5e Mon Sep 17 00:00:00 2001 From: swazrgb <65694696+swazrgb@users.noreply.github.com> Date: Thu, 11 Jun 2020 04:53:36 +0200 Subject: [PATCH 1/2] Provide a scaling ExecutorService and use it from plugin managers The new ExecutorService is configured to consume up to 2*CPU threads and will queue tasks when all threads are busy. Threads are created as needed, and will be disposed of after 1 minute of inactivity. --- .../net/runelite/client/RuneLiteModule.java | 33 +++++- .../client/plugins/ExternalPluginManager.java | 81 +++++-------- .../client/plugins/PluginManager.java | 25 +--- ...heduledExecutorServiceExceptionLogger.java | 109 ++++++++++++++++++ .../client/plugins/PluginManagerTest.java | 25 +++- 5 files changed, 197 insertions(+), 76 deletions(-) create mode 100644 runelite-client/src/main/java/net/runelite/client/util/NonScheduledExecutorServiceExceptionLogger.java diff --git a/runelite-client/src/main/java/net/runelite/client/RuneLiteModule.java b/runelite-client/src/main/java/net/runelite/client/RuneLiteModule.java index d6417c0a00..62e20be0d7 100644 --- a/runelite-client/src/main/java/net/runelite/client/RuneLiteModule.java +++ b/runelite-client/src/main/java/net/runelite/client/RuneLiteModule.java @@ -24,13 +24,18 @@ */ package net.runelite.client; +import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.inject.AbstractModule; import com.google.inject.Provides; import com.google.inject.name.Names; import java.applet.Applet; import java.io.File; +import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; import java.util.function.Supplier; import javax.annotation.Nullable; import javax.inject.Singleton; @@ -51,6 +56,7 @@ import net.runelite.client.plugins.PluginManager; import net.runelite.client.task.Scheduler; import net.runelite.client.util.DeferredEventBus; import net.runelite.client.util.ExecutorServiceExceptionLogger; +import net.runelite.client.util.NonScheduledExecutorServiceExceptionLogger; import net.runelite.http.api.RuneLiteAPI; import okhttp3.Cache; import okhttp3.OkHttpClient; @@ -74,7 +80,6 @@ public class RuneLiteModule extends AbstractModule protected void configure() { bind(File.class).annotatedWith(Names.named("config")).toInstance(config); - bind(ScheduledExecutorService.class).toInstance(new ExecutorServiceExceptionLogger(Executors.newSingleThreadScheduledExecutor())); bind(OkHttpClient.class).toInstance(RuneLiteAPI.CLIENT.newBuilder() .cache(new Cache(new File(RuneLite.CACHE_DIR, "okhttp"), MAX_OKHTTP_CACHE_SIZE)) .build()); @@ -140,4 +145,30 @@ public class RuneLiteModule extends AbstractModule { return configManager.getConfig(LauncherConfig.class); } + + @Provides + @Singleton + ScheduledExecutorService provideScheduledExecutorService() + { + return new ExecutorServiceExceptionLogger(Executors.newSingleThreadScheduledExecutor(new ThreadFactoryBuilder() + .setNameFormat("scheduled-%d") + .build())); + } + + @Provides + @Singleton + ExecutorService provideExecutorService() + { + int poolSize = 2 * Runtime.getRuntime().availableProcessors(); + + // Will start up to poolSize threads (because of allowCoreThreadTimeOut) as necessary, and times out + // unused threads after 1 minute + ThreadPoolExecutor executor = new ThreadPoolExecutor(poolSize, poolSize, + 60L, TimeUnit.SECONDS, + new LinkedBlockingQueue<>(), + new ThreadFactoryBuilder().setNameFormat("worker-%d").build()); + executor.allowCoreThreadTimeOut(true); + + return new NonScheduledExecutorServiceExceptionLogger(executor); + } } diff --git a/runelite-client/src/main/java/net/runelite/client/plugins/ExternalPluginManager.java b/runelite-client/src/main/java/net/runelite/client/plugins/ExternalPluginManager.java index 27c8dd2629..dbe9d3aa1b 100644 --- a/runelite-client/src/main/java/net/runelite/client/plugins/ExternalPluginManager.java +++ b/runelite-client/src/main/java/net/runelite/client/plugins/ExternalPluginManager.java @@ -28,7 +28,6 @@ import com.google.common.collect.Lists; import com.google.common.graph.GraphBuilder; import com.google.common.graph.Graphs; import com.google.common.graph.MutableGraph; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.inject.Binder; import com.google.inject.CreationException; import com.google.inject.Injector; @@ -48,7 +47,6 @@ import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -103,6 +101,7 @@ public class ExternalPluginManager private final List repositories = new ArrayList<>(); private final OpenOSRSConfig openOSRSConfig; private final EventBus eventBus; + private final ExecutorService executorService; private final ConfigManager configManager; private final Map pluginsMap = new HashMap<>(); @Getter(AccessLevel.PUBLIC) @@ -119,12 +118,14 @@ public class ExternalPluginManager PluginManager pluginManager, OpenOSRSConfig openOSRSConfig, EventBus eventBus, + ExecutorService executorService, ConfigManager configManager, Groups groups) { this.runelitePluginManager = pluginManager; this.openOSRSConfig = openOSRSConfig; this.eventBus = eventBus; + this.executorService = executorService; this.configManager = configManager; this.groups = groups; @@ -465,62 +466,44 @@ public class ExternalPluginManager final long start = System.currentTimeMillis(); - // some plugins get stuck on IO, so add some extra threads - ExecutorService exec = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2, - new ThreadFactoryBuilder() - .setNameFormat("external-plugin-manager-%d") - .build()); - - try + List scannedPlugins = new CopyOnWriteArrayList<>(); + sortedPlugins.forEach(group -> { - List scannedPlugins = new CopyOnWriteArrayList<>(); - sortedPlugins.forEach(group -> - { - List> curGroup = new ArrayList<>(); - group.forEach(pluginClazz -> - curGroup.add(exec.submit(() -> - { - Plugin plugininst; - try - { - //noinspection unchecked - plugininst = instantiate(scannedPlugins, (Class) pluginClazz, init, initConfig); - scannedPlugins.add(plugininst); - } - catch (PluginInstantiationException e) - { - log.warn("Error instantiating plugin!", e); - return; - } - - loaded.getAndIncrement(); - - RuneLiteSplashScreen.stage(.67, .75, "Loading external plugins", loaded.get(), scannedPlugins.size()); - }))); - curGroup.forEach(future -> + List> curGroup = new ArrayList<>(); + group.forEach(pluginClazz -> + curGroup.add(executorService.submit(() -> { + Plugin plugininst; try { - future.get(); + //noinspection unchecked + plugininst = instantiate(scannedPlugins, (Class) pluginClazz, init, initConfig); + scannedPlugins.add(plugininst); } - catch (InterruptedException | ExecutionException e) + catch (PluginInstantiationException e) { - log.warn("Could not instantiate external plugin", e); + log.warn("Error instantiating plugin!", e); + return; } - }); - }); - log.info("External plugin instantiation took {}ms", System.currentTimeMillis() - start); - } - finally - { - List unfinishedTasks = exec.shutdownNow(); - if (!unfinishedTasks.isEmpty()) + loaded.getAndIncrement(); + + RuneLiteSplashScreen.stage(.67, .75, "Loading external plugins", loaded.get(), scannedPlugins.size()); + }))); + curGroup.forEach(future -> { - // This shouldn't happen since we Future#get all tasks submitted to the executor - log.warn("Did not complete all update tasks: {}", unfinishedTasks); - } - } + try + { + future.get(); + } + catch (InterruptedException | ExecutionException e) + { + log.warn("Could not instantiate external plugin", e); + } + }); + }); + + log.info("External plugin instantiation took {}ms", System.currentTimeMillis() - start); } @SuppressWarnings("unchecked") diff --git a/runelite-client/src/main/java/net/runelite/client/plugins/PluginManager.java b/runelite-client/src/main/java/net/runelite/client/plugins/PluginManager.java index 5788b916f3..a5ac0465b7 100644 --- a/runelite-client/src/main/java/net/runelite/client/plugins/PluginManager.java +++ b/runelite-client/src/main/java/net/runelite/client/plugins/PluginManager.java @@ -34,7 +34,6 @@ import com.google.common.graph.Graphs; import com.google.common.graph.MutableGraph; import com.google.common.reflect.ClassPath; import com.google.common.reflect.ClassPath.ClassInfo; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.inject.Binder; import com.google.inject.CreationException; import com.google.inject.Injector; @@ -54,7 +53,6 @@ import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; @@ -96,6 +94,7 @@ public class PluginManager private final EventBus eventBus; private final Scheduler scheduler; + private final ExecutorService executorService; private final ConfigManager configManager; private final Provider sceneTileManager; private final List plugins = new CopyOnWriteArrayList<>(); @@ -115,6 +114,7 @@ public class PluginManager PluginManager( final EventBus eventBus, final Scheduler scheduler, + final ExecutorService executorService, final ConfigManager configManager, final Provider sceneTileManager, final Groups groups, @@ -122,6 +122,7 @@ public class PluginManager { this.eventBus = eventBus; this.scheduler = scheduler; + this.executorService = executorService; this.configManager = configManager; this.sceneTileManager = sceneTileManager; this.groups = groups; @@ -401,20 +402,12 @@ public class PluginManager final long start = System.currentTimeMillis(); - // some plugins get stuck on IO, so add some extra threads - ExecutorService exec = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2, - new ThreadFactoryBuilder() - .setNameFormat("plugin-manager-%d") - .build()); - - try - { List scannedPlugins = new CopyOnWriteArrayList<>(); sortedPlugins.forEach(group -> { List> curGroup = new ArrayList<>(); group.forEach(pluginClazz -> - curGroup.add(exec.submit(() -> + curGroup.add(executorService.submit(() -> { Plugin plugin; try @@ -447,16 +440,6 @@ public class PluginManager log.info("Plugin instantiation took {}ms", System.currentTimeMillis() - start); return scannedPlugins; - } - finally - { - List unfinishedTasks = exec.shutdownNow(); - if (!unfinishedTasks.isEmpty()) - { - // This shouldn't happen since we Future#get all tasks submitted to the executor - log.warn("Did not complete all update tasks: {}", unfinishedTasks); - } - } } public boolean startPlugin(Plugin plugin) throws PluginInstantiationException diff --git a/runelite-client/src/main/java/net/runelite/client/util/NonScheduledExecutorServiceExceptionLogger.java b/runelite-client/src/main/java/net/runelite/client/util/NonScheduledExecutorServiceExceptionLogger.java new file mode 100644 index 0000000000..002969bf95 --- /dev/null +++ b/runelite-client/src/main/java/net/runelite/client/util/NonScheduledExecutorServiceExceptionLogger.java @@ -0,0 +1,109 @@ +package net.runelite.client.util; + +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import static net.runelite.client.util.RunnableExceptionLogger.wrap; +import org.jetbrains.annotations.NotNull; + +// Awkward name because plugins already referenced the ExecutorServiceExceptionLogger +// (which only handles ScheduledExecutorServices) before this class was introduced +public class NonScheduledExecutorServiceExceptionLogger implements ExecutorService +{ + private final ExecutorService service; + + public NonScheduledExecutorServiceExceptionLogger(ExecutorService service) + { + this.service = service; + } + + @Override + public void shutdown() + { + service.shutdown(); + } + + @NotNull + @Override + public List shutdownNow() + { + return service.shutdownNow(); + } + + @Override + public boolean isShutdown() + { + return service.isShutdown(); + } + + @Override + public boolean isTerminated() + { + return service.isTerminated(); + } + + @Override + public boolean awaitTermination(long timeout, @NotNull TimeUnit unit) throws InterruptedException + { + return service.awaitTermination(timeout, unit); + } + + @Override + public void execute(@NotNull Runnable command) + { + service.execute(wrap(command)); + } + + @NotNull + @Override + public Future submit(@NotNull Callable task) + { + return service.submit(CallableExceptionLogger.wrap(task)); + } + + @NotNull + @Override + public Future submit(@NotNull Runnable task, T result) + { + return service.submit(wrap(task), result); + } + + @NotNull + @Override + public Future submit(@NotNull Runnable task) + { + return service.submit(wrap(task)); + } + + @NotNull + @Override + public List> invokeAll(@NotNull Collection> tasks) throws InterruptedException + { + return service.invokeAll(tasks); + } + + @NotNull + @Override + public List> invokeAll(@NotNull Collection> tasks, long timeout, @NotNull TimeUnit unit) throws InterruptedException + { + return service.invokeAll(tasks, timeout, unit); + } + + @NotNull + @Override + public T invokeAny(@NotNull Collection> tasks) throws InterruptedException, ExecutionException + { + return service.invokeAny(tasks); + } + + @Override + public T invokeAny(@NotNull Collection> tasks, long timeout, @NotNull TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException + { + return service.invokeAny(tasks, timeout, unit); + } +} diff --git a/runelite-client/src/test/java/net/runelite/client/plugins/PluginManagerTest.java b/runelite-client/src/test/java/net/runelite/client/plugins/PluginManagerTest.java index d2a7ae6636..b60385d88a 100644 --- a/runelite-client/src/test/java/net/runelite/client/plugins/PluginManagerTest.java +++ b/runelite-client/src/test/java/net/runelite/client/plugins/PluginManagerTest.java @@ -47,6 +47,8 @@ import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import net.runelite.api.Client; import net.runelite.api.events.Event; import net.runelite.client.RuneLite; @@ -56,6 +58,7 @@ import net.runelite.client.config.ConfigItem; import net.runelite.client.eventbus.AccessorGenerator; import net.runelite.client.eventbus.EventBus; import net.runelite.client.eventbus.Subscribe; +import org.junit.After; import static org.junit.Assert.assertEquals; import org.junit.Before; import org.junit.Rule; @@ -81,12 +84,16 @@ public class PluginManagerTest @Bind public Client client; + private ExecutorService executorService; + private Set pluginClasses; private Set configClasses; @Before public void before() throws IOException { + executorService = Executors.newSingleThreadScheduledExecutor(); + Injector injector = Guice.createInjector(Modules .override(new RuneLiteModule(() -> null, RuneLite.DEFAULT_CONFIG_FILE)) .with(BoundFieldModule.of(this))); @@ -114,10 +121,16 @@ public class PluginManagerTest } } + @After + public void after() + { + executorService.shutdownNow(); + } + @Test public void testLoadPlugins() throws Exception { - PluginManager pluginManager = new PluginManager(null, null, null, null, null, null); + PluginManager pluginManager = new PluginManager(null, null, executorService, null, null, null, null); pluginManager.setOutdated(true); pluginManager.loadCorePlugins(); Collection plugins = pluginManager.getPlugins(); @@ -128,7 +141,7 @@ public class PluginManagerTest .count(); assertEquals(expected, plugins.size()); - pluginManager = new PluginManager(null, null, null, null, null, null); + pluginManager = new PluginManager(null, null, executorService, null, null, null, null); pluginManager.loadCorePlugins(); plugins = pluginManager.getPlugins(); @@ -146,7 +159,7 @@ public class PluginManagerTest modules.add(new GraphvizModule()); modules.add(new RuneLiteModule(() -> null, RuneLite.DEFAULT_CONFIG_FILE)); - PluginManager pluginManager = new PluginManager(null, null, null, null, null, null); + PluginManager pluginManager = new PluginManager(null, null, executorService, null, null, null, null); pluginManager.loadCorePlugins(); modules.addAll(pluginManager.getPlugins()); @@ -198,7 +211,7 @@ public class PluginManagerTest public void testEventbusAnnotations() throws Exception { EventBus eventbus = new EventBus(); - PluginManager pluginManager = new PluginManager(eventbus, null, null, null, null, null) + PluginManager pluginManager = new PluginManager(eventbus, null, executorService, null, null, null, null) { @Override public boolean isPluginEnabled(Plugin plugin) @@ -207,7 +220,9 @@ public class PluginManagerTest } }; - class TestEvent implements Event {} + class TestEvent implements Event + { + } class TestPlugin extends Plugin { private boolean thisShouldBeTrue = false; From 9d9f7ff18b4c7377d278558f3db8259771123d02 Mon Sep 17 00:00:00 2001 From: swazrgb <65694696+swazrgb@users.noreply.github.com> Date: Thu, 11 Jun 2020 05:03:58 +0200 Subject: [PATCH 2/2] Shutdown ExecutorService during CLientUI shutdown --- .../java/net/runelite/client/ui/ClientUI.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java b/runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java index 83fb860185..63e3196f06 100644 --- a/runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java +++ b/runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java @@ -51,6 +51,8 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.time.Duration; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import javax.inject.Inject; import javax.inject.Provider; @@ -134,6 +136,7 @@ public class ClientUI private final MouseManager mouseManager; private final Applet client; private final ConfigManager configManager; + private final ExecutorService executorService; private final Provider clientThreadProvider; private final EventBus eventBus; private final CardLayout cardLayout = new CardLayout(); @@ -161,6 +164,7 @@ public class ClientUI MouseManager mouseManager, @Nullable Applet client, ConfigManager configManager, + ExecutorService executorService, Provider clientThreadProvider, EventBus eventbus) { @@ -169,6 +173,7 @@ public class ClientUI this.mouseManager = mouseManager; this.client = client; this.configManager = configManager; + this.executorService = executorService; this.clientThreadProvider = clientThreadProvider; this.eventBus = eventbus; @@ -601,9 +606,21 @@ public class ClientUI saveClientBoundsConfig(); ClientShutdown csev = new ClientShutdown(); eventBus.post(ClientShutdown.class, csev); + executorService.shutdown(); + new Thread(() -> { csev.waitForAllConsumers(Duration.ofSeconds(10)); + try + { + if (!executorService.awaitTermination(5, TimeUnit.SECONDS)) + { + executorService.shutdownNow(); + } + } + catch (InterruptedException ignored) + { + } if (client != null) {