SpongePowered / SpongeAPI

A Minecraft plugin API
http://www.spongepowered.org/
MIT License
1.14k stars 343 forks source link

Single threaded asynchronous executor suggestion #1613

Open KaiKikuchi opened 7 years ago

KaiKikuchi commented 7 years ago

Currently, Sponge.getScheduler().createAsyncExecutor(plugin) seems to wrap a Executors.newCachedThreadPool().

This forces plugin developers to rely on Executors.newSingleThreadExecutor() instead of the SpongeAPI's asynchronous scheduler, like in this case.

Although it's not strictly necessary to add this to the API, I think it would be a nice addition, especially for those who want to rely on the SpongeAPI as much as possible.

Cybermaxke commented 7 years ago

Can't we just limit the amount of Threads that can be created by the ExecutorService?

ExecutorService service = new ThreadPoolExecutor(0, maximumPoolSize, 60L, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(), threadFactory); When calling Executors.newCachedThreadPool() is the maximumPoolSize set to Integer.MAX_VALUE, all the other values are taken from Executors.

KaiKikuchi commented 7 years ago

Yes, definitely.

The Executors.newSingleThreadExecutor() is, for the most part, just a Executors.newFixedThreadPool(1), as stated on the [Javadoc](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor()).

The only thing that I am not sure about is [this part](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor()):

Tasks are guaranteed to execute sequentially, and no more than one task will be active at any given time.

Will a Executors.newCachedThreadPool(1) execute tasks sequentially..?

Cybermaxke commented 7 years ago

Yes, they will also be executed sequentially. Because there is only one Thread to execute the tasks, so everything has to be executed on that thread. Not sure why you would create a CachedThreadPool in this case.

KaiKikuchi commented 7 years ago

No, actually Executors.newCachedThreadPool(1) is not even a thing! Honestly I am unsure of the real usefulness of using a CachedThreadPool on Sponge, but for a single thread pool it has to be a FixedThreadPool.

Cybermaxke commented 7 years ago

A CachedThreadPool cleans up Threads when they are no longer used, a FixedThreadPool will keep them all active even though they are not used. I would use a CachedThreadPool, but limit the maximum amount of Threads that can be created.

KaiKikuchi commented 7 years ago

I think that a custom implementation of a FixedThreadPool is necessary.

Anyway, this was just a suggestion, I'm sure you will figure out how to limit the number of threads (as it's not possible with the current CachedThreadPool implementation) and avoid resource issues.

Also please let it respect the order of insertion, as it would be nice in a lot of cases, like I/O operations.

Cybermaxke commented 7 years ago

You can easily limit the amount of maximum threads, you just have to use the constructor instead of the helper methods in Executors. ExecutorService service = new ThreadPoolExecutor(0, maximumPoolSize, 60L, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(), threadFactory);

KaiKikuchi commented 7 years ago

Actually I don't think a CachedThreadPool is a good idea.

I checked how the CachedThreadPool is implemented, and basically it'll discard any thread that is waiting for more than 60 seconds. This means that this is not a good idea if you want to execute stuff that take a long time to complete.

keepAliveTime: when the number of threads is greater than the core, this is the maximum time that excess idle threads will wait for new tasks before terminating.

I would keep using a Executors.newSingleThreadExecutor() in some case.

Ignore this. I think I misunderstood. It's about threads, not tasks.

KaiKikuchi commented 7 years ago

A SynchronousQueue cannot be used, because it would not queue the tasks. A RejectedExecutionException is thrown if you try to submit a task to the ThreadPoolExecutor you wrote above if the pool is full.

I would use a ExecutorService service = new ThreadPoolExecutor(0, maximumPoolSize, 60L, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(), threadFactory); instead, so it would remove the unused threads if the ThreadPoolExecutor is not used, and you can queue any amount of long-execution tasks.

Also it would be cool if we could construct a SpongeExecutorService from an ExecutorService, so developers could use their own ExecutorService implementations without losing all the additional things added by the SpongeExecutorService.

ryantheleach commented 6 years ago

@SpongePowered/developers Someone with more experience with the scheduler needs to chime in and come up with either a reason to close, or a plan of what we will do to resolve this. Please note the discussion here: https://github.com/MinecraftPortCentral/GriefPrevention/pull/415

amaranth commented 6 years ago

Seems like a job for ForkJoinPool.commonPool(). It should only ever give RejectedExecutionException if you try to add a task while the VM is shutting down and by default uses numCores - 1 max threads unless you override it via the java.util.concurrent.ForkJoinPool.common.parallelism property. It's also created on VM startup (or rather the first time the class is loaded but it gets used all over the place) so you shouldn't have to worry about it failing.

If you insist on custom thread names so need a separate pool just make one that works like commonPool() and use it. The key is to only make one and have everything in Sponge use it.