Terasology icon indicating copy to clipboard operation
Terasology copied to clipboard

Refactor usage of concurrency with reactor

Open pollend opened this issue 3 years ago • 14 comments

Terasology does not use or promote a consistent feature set or technologies for handling concurrency.

In some situations the number of threads are over-provisioned, and in some cases its under-provisioned. Some workloads might benefit from more threads but selectively changing the number of threads by hand will lead to over-provisioning on some systems and under-provisioning on another system.

Poor utilization of threads will end up with situations where the kernel is switching between different tasks. If every thread is a CPU bound task then the processor will spend a lot of time context switching between the different tasks.

We propose dropping ThreadManagerSubsystem and use the functionality provided by Project Reactor.

Currently, TaskMaster is used to push tasks to an off thread where the use case of TaskMaster is probably not needed and the spawned thread is mostly unused.

Current situation

  • Chunk-Unloader (allocate 4 threads)

    • int list to queue event for BeforeDeactivateBlocks
    • very light load so threads are mostly idle
    • at any moment a single thread will become active
  • Chunk-Updater (allocates 8 threads)

    • generates chunk mesh and queue them back on the main thread
  • LOD Chunk Generation

  • only triggered for handling different levels of lod

  • ThreadManagerSubsystem (allocates 16 threads) only used for a couple situations and the fixed number of threads does not properly utilize the number of cores. and hardly used so all these threads are idle.

    • loadingGame
    • create new world
    • saving screenshot
  • TaskMaster

    • Behaviors (1 thread)
    • FlexiablePathFinding (4 threads)
    • Pathfinding (1 thread)

  • [x] drop usage of the ThreadManagerSubsystem easily can be replaced by usage of Observable
  • [x] chunk Mesh generator https://github.com/MovingBlocks/Terasology/pull/4786
  • [ ] remove usage of TaskMaster
    • [x] #4800
    • [x] #4786
    • [x] https://github.com/Terasology/Behaviors/pull/77
    • [ ] https://github.com/Terasology/FlexiblePathfinding/pull/24
    • [x] ~~https://github.com/Terasology/Pathfinding/pull/62~~ — module removed
    • [ ] chunk unloader probably does not benefit with parallelism
  • [ ] check whether Reactor threads need to be explicitly configured as daemons so they don't prevent the process from closing if they get stuck
  • [ ] LOD Chunk generation uses a custom set of threads easily replaced with logic in this PR (https://github.com/MovingBlocks/Terasology/pull/4786) . we need to do something slightly different because constantly replacing the static mesh is un-optimal and produces a lot of extra traffic between the GPU and CPU.

pollend avatar Jun 26 '21 19:06 pollend

Do we have anyone familiar enough with the various implementations of the reactive pattern in Java to evaluate Project Reactor (with Flux and Mono APIs) in comparison to RxJava2?

keturn avatar Jun 27 '21 17:06 keturn

I tried to sketch out what we're currently doing and using (no guarantee for completeness): image

skaldarnar avatar Jun 27 '21 18:06 skaldarnar

@skaldarnar that's right got me confused because ExecutorService just creates a pool of threads and each instance of TaskMaster will introduce its own set of threads. we might get into a situation where the processor is constantly context switching between a lot of computational threads.

pollend avatar Jun 27 '21 18:06 pollend

Do we have anyone familiar enough with the various implementations of the reactive pattern in Java to evaluate Project Reactor (with Flux and Mono APIs) in comparison to RxJava2?

there are a lot of weird facilities to map one to the other and vice versa. probably want to provide one use case so we don't end up juggling a lot of these reactive libraries.

pollend avatar Jun 27 '21 18:06 pollend

@pollend I've updated your initial post by adding links to ReactiveX concepts and tweaking the sentences a bit here and there. I hope I did not change the semantic anywhere.

got me confused because ExecutorService just creates a pool of threads and each instance of TaskMaster will introduce its own set of threads

You plan forward sounds good to me on first read. I just wanted to share that picture I made, where we can hopefully remove a few of the green boxes in the future, and replace others with RX concepts.

skaldarnar avatar Jun 27 '21 18:06 skaldarnar

https://github.com/MovingBlocks/Terasology/pull/4799 @skaldarnar this should correctly expose reactivex to module space. you need to use the GameThread wrapper though :/

pollend avatar Jun 27 '21 19:06 pollend

I'd like the roadmap to include a plan for replacing the debug thread view; currently ThreadMonitorPanel and the RunningThreads metrics mode use ThreadMonitor.

https://github.com/MovingBlocks/Terasology/issues/4637 links to a few options that work with ExecutorServices. At the time, I wasn't checking for integration with RxJava or Project Reactor. They might have something built-in, or a different integration option for these other libraries.

keturn avatar Jun 27 '21 20:06 keturn

I'd like the roadmap to include a plan for replacing the debug thread view; currently ThreadMonitorPanel and the RunningThreads metrics mode use ThreadMonitor.

#4637 links to a few options that work with ExecutorServices. At the time, I wasn't checking for integration with RxJava or Project Reactor. They might have something built-in, or a different integration option for these other libraries.

hmm, would be a nice way to do some kind of server metrics. processing results from the running tasks?

pollend avatar Jun 27 '21 20:06 pollend

some main problems with RXJava. There is a table of mappings that define a flowable, Observable, Single, Maybe, and Completable and mappings from one to the other. 0..N defined for Floablw and Observable but only flowable has backpressure. Single is one element only but Maybe is 0 or 1. Completable is 0 element or error. there are different methods to map one type to another. the inconsistent API is difficult to follow. The mappings make this pretty confusing where reactor has Flux and Mono. Flux is 0...N and mono is just a single object. I like this a lot better then the former.

While working with Flowable I managed to get into a situation where objects getting pushed to my Flowable were not getting handled by the subscriber where this pattern seems to be more reliable with reactor. I was looking at this benchmark comparing the two and found the reactor implementation is quite a bit faster: https://github.com/MovingBlocks/Terasology/pull/4786

https://github.com/akarnokd/akarnokd-misc/issues/7

pollend avatar Jul 19 '21 04:07 pollend

Reactor explicitly has built-in integration with micrometer, which makes for a good default answer to which of the metrics implementations in #4637 we should use.

keturn avatar Jul 27 '21 00:07 keturn

Reactor explicitly has built-in integration with micrometer, which makes for a good default answer to which of the metrics implementations in #4637 we should use.

me and @DarkWeird were discussing this.

pollend avatar Jul 27 '21 03:07 pollend

We briefly discussed this in today's org meeting and I quickly updated the graphic to match the current state of things:

untitled_page_1

skaldarnar avatar Aug 28 '21 16:08 skaldarnar

I should refer my pr there #4865

DarkWeird avatar Aug 29 '21 09:08 DarkWeird

And i have a several thinks about networking via reactor. Reactor project have reactor-netty lib:

  1. network subsystem should looks smaller and easier with it.
  2. We can use HttpClient from reactor-netty. It should make module downloader and ModuleListDownload more faster(?) And non blocking (especially for ModuleListDownloader, which use separate thread via new Thread :( )
  3. We can make refactor of DiscordRPC. (-1 thread)

DarkWeird avatar Aug 29 '21 09:08 DarkWeird