Add thread manager crate to agave #3890

alexpyattaev · 2024-12-03T16:10:57Z

Problem

There exist a variety of perf issues related to unorganized thread pools that spawn far more threads than are useful on a given machine, this was confirmed by measurements (see measurement examples in thread-manager/examples directory) and is relatively easy to fix in agave.

Idea is to eventually address the needs of

Summary of Changes

Added new agave-thread-manager crate, which is to be gradually hooked into the agave itself. It will be used to centralize thread pool creation such that their core allocations can be controlled, and total thread count can gradually be reduced to match core count as closely as possible. Crate comes with its own benchmarks and some tests. The crate has full functionality only on Linux as this is the only platform where it is relevant.

KirillLykov · 2024-12-12T09:02:26Z

thread-manager/examples/core_contention_basics.rs

@@ -48,11 +48,12 @@ fn main() -> anyhow::Result<()> {
        println!("Running {exp}");
        let mut conffile = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
        conffile.push(exp);
-        let conffile = std::fs::File::open(conffile)?;


Typically, we don't specify configurations in files. Doing it in code instead for the example/test cases, or with command line flags for other cases. Sometimes use yml (for accounts information for example).

I got you, you want to read thread-manager/examples/core_contention_contending_set.toml with it.
Yeah, could you maybe have a method that reads this file and used in both example and production environment in the future?

this file will never be read in prod, its just for examples which are also benchmarks/tests. I'm not even sure we will use toml in prod for this yet, though it is likely.

thread-manager/examples/core_contention_basics.rs

KirillLykov · 2024-12-12T09:08:31Z

I think you need to explain broader audience proposed changes in the context. Maybe by setting up a meeting with involved parties.

alexpyattaev · 2024-12-12T14:06:47Z

Yes, a meeting would be necessary eventually, but I also need the code to be reasonably good before a million people look at it=)

bw-solana

I like the high level idea. Left a few nits and some questions.
I need to better understand how you envision the string-->string-->runtime mapping working before I can weigh in on it

thread-manager/README.md

bw-solana · 2025-01-09T17:14:56Z

thread-manager/README.md

+## Scheduling policy and priority
+If you want you can set thread scheduling policy and priority. Keep in mind that this will likely require
+```bash
+ sudo setcap cap_sys_nice+ep


in the past we've had tuning scripts that could handle recommended configurations like this. If it makes sense, we could add this wherever relevant

I'm not sure about priority stuff at this point, it is super platform specific and mostly a placeholder at the moment, as getting this past CI will be unfun (due to extra priveleges necessary)

thread-manager/src/tokio_runtime.rs

bw-solana · 2025-01-09T17:34:29Z

thread-manager/src/lib.rs

+#[derive(Default, Debug)]
+pub struct ThreadManagerInner {
+    pub tokio_runtimes: HashMap<ConstString, TokioRuntime>,
+    pub tokio_runtime_mapping: HashMap<ConstString, ConstString>,


is the idea here that we essentially have a mapping like so:
service --> tokio name --> tokio runtime
And service --> tokio name could many to 1 but the tokio name --> tokio runtime is 1 to 1?

yes, exactly! I believe we will eventually have maybe 1-2 tokio runtimes + 2-3 rayons. and mappings to those.

cool! I think it makes sense. Maybe we can get rid of the intermediate mapping and go just from service to runtime (I believe you mentioned this elsewhere)

thread-manager/src/lib.rs

bw-solana · 2025-01-09T17:47:23Z

thread-manager/src/lib.rs

+    ) -> Option<&'a T> {
+        match mapping.get(name) {
+            Some(n) => runtimes.get(n),
+            None => match mapping.get("default") {


I'm wondering if we could check some of these mappings up front since we only create/map these thread pools one time at startup. This might make runtime simpler.

For example, when creating ThreadManager, we iterate through the mappings, and if string1 maps to a non-existent string2, we log a warning and then map it to "default".

Then maybe we could throw an error here if we try to get a runtime that doesn't exist

I guess we could, but then we'd need a complete list of all needed runtimes which would be a nightmare to maintain... Mapping to "default" is definitely doable.

bw-solana · 2025-01-09T17:47:38Z

thread-manager/src/lib.rs

+    }
+
+    /*  #[test]
+    fn thread_priority() {


why is this one commented out?

This needs root/extra capabilities + realtime kernel features to really be useful. So it is set aside for "future use" for now.

might be nice to add that context into the code. We could also just mark this #[ignore]

thread-manager/src/lib.rs

bw-solana · 2025-01-09T17:52:40Z

Adding @alessandrod as I'm sure he'll have opinions on this 😄

alexpyattaev · 2025-01-09T21:28:32Z

I like the high level idea. Left a few nits and some questions. I need to better understand how you envision the string-->string-->runtime mapping working before I can weigh in on it
runtime mapping is supposed to work as follows:

in code, we ask for a named pool, identified by its purpose (e.g. solGossipConsume).
In config, we define that solGossipConsume should be handled by RayonSigVerify pool
Further in config, we also define that RayonSigVerify shoudl have 16 threads pinned to cores 32-48 and should have certain priority w.r.t. other threads etc etc.

From this idea we get to solGossipConsume -> RayonSigVerify -> ThreadPool sort of mapping. At least that was the idea. We can dispense with the intermediate layer but then each pool would necessarily run on its own set of threads which may not be ideal. We can, of course, axe this functionality and bring it back later as appropriate.

thread-manager/README.md

thread-manager/src/policy.rs

thread-manager/src/native_thread_runtime.rs

thread-manager/src/rayon_runtime.rs

apfitzge · 2025-01-09T22:09:38Z

thread-manager/src/tokio_runtime.rs

+    }
+
+    /// Makes test runtime with 2 threads, only for unittests
+    pub fn new_for_tests() -> Self {


#[cfg(test)]?

No, can not be done as it is to be used from other crates.

For this use case, our convention, instead, to use

#[cfg(feature = "dev-context-only-utils")]

and to Cargo file

[features] dev-context-only-utils = []

thread-manager/src/tokio_runtime.rs

apfitzge · 2025-01-09T22:46:36Z

thread-manager/src/lib.rs

+    rayon_runtime::{RayonConfig, RayonRuntime},
+    tokio_runtime::{TokioConfig, TokioRuntime},
+};
+pub type ConstString = Box<str>;


Why do we want boxed strings? it seems like this is the whole cause of us not being able to simply clone the maps, but doesn't seem necessary to me at first glance.

To make sure noone gets ideas about mutating them. But I guess they can be just String if that is somehow better. We would not be able to just clone the maps anyway, but I guess it would make code slightly cleaner. I'll check.

Box's don't prevent mutation though. Just provide unique ownership

Fair enough. Should I change to String?

Yeah I think unless we have a good reason to use box we shouldn't

Co-authored-by: Andrew Fitzgerald <[email protected]>

bw-solana

Left another few nits, but overall LGTM!

Would like others to weigh in on design at least before merging as this will obviously be foundational to how we manage threads, and once we start actually integrating w/ services, will become harder to change things

thread-manager/README.md

thread-manager/examples/core_contention_basics.rs

alexpyattaev force-pushed the thread_manager branch 3 times, most recently from da038b3 to 309350b Compare December 8, 2024 07:50

alexpyattaev marked this pull request as ready for review December 8, 2024 22:05

alexpyattaev requested review from KirillLykov and steviez December 8, 2024 22:06

KirillLykov reviewed Dec 12, 2024

View reviewed changes

thread-manager/examples/core_contention_basics.rs Outdated Show resolved Hide resolved

KirillLykov reviewed Dec 12, 2024

View reviewed changes

thread-manager/examples/core_contention_basics.rs Outdated Show resolved Hide resolved

alexpyattaev force-pushed the thread_manager branch 2 times, most recently from 2c6a9dd to 4e1d728 Compare December 17, 2024 21:56

alexpyattaev force-pushed the thread_manager branch from c93751c to 60a04f8 Compare January 8, 2025 22:03

alexpyattaev requested a review from bw-solana January 8, 2025 22:04

alexpyattaev force-pushed the thread_manager branch 2 times, most recently from 895fd15 to e00029a Compare January 8, 2025 22:47

bw-solana reviewed Jan 9, 2025

View reviewed changes

bw-solana requested a review from alessandrod January 9, 2025 17:52