Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor usage of concurrency with reactor #4798

Open
6 of 11 tasks
pollend opened this issue Jun 26, 2021 · 14 comments
Open
6 of 11 tasks

Refactor usage of concurrency with reactor #4798

pollend opened this issue Jun 26, 2021 · 14 comments
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Topic: Concurrency Requests, issues, and changes relating to threading, concurrency, parallel execution, etc. Type: Improvement Request for or addition/enhancement of a feature

Comments

@pollend
Copy link
Member

pollend commented Jun 26, 2021

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)

@pollend pollend changed the title refactor usage of concurrency with ReactiveX Refactor usage of concurrency with ReactiveX Jun 26, 2021
@keturn
Copy link
Member

keturn commented Jun 27, 2021

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?

@skaldarnar
Copy link
Member

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

@pollend
Copy link
Member Author

pollend commented Jun 27, 2021

@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
Copy link
Member Author

pollend commented Jun 27, 2021

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.

@skaldarnar
Copy link
Member

@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.

@pollend
Copy link
Member Author

pollend commented Jun 27, 2021

#4799 @skaldarnar this should correctly expose reactivex to module space. you need to use the GameThread wrapper though :/

@keturn
Copy link
Member

keturn commented Jun 27, 2021

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.

@pollend
Copy link
Member Author

pollend commented Jun 27, 2021

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 added a commit to Terasology/Behaviors that referenced this issue Jun 27, 2021
pollend added a commit to Terasology/FlexiblePathfinding that referenced this issue Jun 27, 2021
@pollend pollend changed the title Refactor usage of concurrency with ReactiveX Refactor usage of concurrency with reactor Jul 19, 2021
@pollend
Copy link
Member Author

pollend commented Jul 19, 2021

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: #4786

akarnokd/akarnokd-misc#7

@keturn keturn added the Topic: Concurrency Requests, issues, and changes relating to threading, concurrency, parallel execution, etc. label Jul 26, 2021
@keturn
Copy link
Member

keturn commented Jul 27, 2021

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 keturn added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature labels Jul 27, 2021
@pollend
Copy link
Member Author

pollend commented Jul 27, 2021

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.

@skaldarnar
Copy link
Member

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

@DarkWeird
Copy link
Contributor

I should refer my pr there #4865

@DarkWeird
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Topic: Concurrency Requests, issues, and changes relating to threading, concurrency, parallel execution, etc. Type: Improvement Request for or addition/enhancement of a feature
Projects
Status: GOALS
Status: 🏗 In progress
Development

No branches or pull requests

4 participants