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

Build custom runtime for configurable worker threads #91

Merged

Conversation

lukasmittag
Copy link
Contributor

to make the worker threads configurable via env varibale and command line. Documentation currently missing!!!!

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 132 lines in your changes missing coverage. Please review.

Project coverage is 50.08%. Comparing base (706a633) to head (e368e68).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
databroker/src/main.rs 0.00% 132 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   50.02%   50.08%   +0.05%     
==========================================
  Files          31       31              
  Lines       11805    11792      -13     
==========================================
  Hits         5906     5906              
+ Misses       5899     5886      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}
}
let cores = available_parallelism().unwrap().get();
let worker_threads: &usize = args.get_one::<usize>("worker-threads").unwrap_or(&cores);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the number of threads provided is higher than the number of threads allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats meant by number of threads allowed? You can spawn more threads but if you do not have more cores than it will block the thread. We could limit it to the cores available but I do not think it is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean like for example, the user has no idea how many cores the test device has and provides a woker-threads parameter higher than the available number of cores.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it is nothing that prevents you from having more threads than cores, but you do not gain anything from it, but most likely you will get a small performance penalty if specifying more than number of cores. I think it makes sense to write some advice in documentation on when it makes sense to have something else than default.

Is a possible use-case that you want to run Databroker with high priority (for minimal latency), but at the same time you do not want it to steal more than a certain share of the capacity and by that reason you state that it only might use X cores.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a value to print something like "Using X cores, Y cores available", either on debug or info level? Info-level if we consider it as relevant info to get when someone has performance problems and we do not want to get a huge debug log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added info message how many threads are spawned and how many cores are available on the system.

Copy link
Contributor

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Did some smoke test using kuksa-client. No problems detected

erik@debian6:~/kuksa-databroker$ cargo run --bin databroker -- --metadata data/vss-core/vss_release_4.0.json --insecure --worker-threads=2000
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/databroker --metadata data/vss-core/vss_release_4.0.json --insecure --worker-threads=2000`
2024-11-05T13:39:01.519395Z  INFO databroker: Init logging from RUST_LOG (environment variable not found)
2024-11-05T13:39:01.519456Z  INFO databroker: Starting Kuksa Databroker 0.4.7-dev.0
2024-11-05T13:39:01.519470Z  INFO databroker: Using 2000 threads with 3 cores available on the system

Copy link
Contributor

@rafaeling rafaeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@erikbosch erikbosch merged commit 0af2e2e into eclipse-kuksa:main Nov 7, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants