Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1364 +/- ##
==========================================
- Coverage 56.79% 55.71% -1.09%
==========================================
Files 140 140
Lines 20106 20100 -6
Branches 4898 4898
==========================================
- Hits 11420 11199 -221
- Misses 5832 6047 +215
Partials 2854 2854
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
docs/3.1-Shadow-Config.md
Outdated
| - [`general.data_directory`](#generaldata_directory) | ||
| - [`general.heartbeat_interval`](#generalheartbeat_interval) | ||
| - [`general.log_level`](#generallog_level) | ||
| - [`general.parallelism`](#general_parallelism) |
There was a problem hiding this comment.
The other options don't have an underscore between the general keyword and the name of the option. I have no idea why not, it's harder to read without the underscore. I assume there was a reason? Do we want to be consistent?
There was a problem hiding this comment.
Good catch! Yeah, we don't control how the anchors are generated for the headings, and unfortunately it omits the '.', so this was a broken link.
| How many parallel threads to use to run the simulation. Optimal performance is | ||
| usually obtained with `nproc`, or sometimes `nproc/2` with hyperthreading. |
There was a problem hiding this comment.
Do we need to explain what happens if set to null? Or maybe this isn't the place for that?
There was a problem hiding this comment.
I don't think there was a way for the user to explicitly set this to NULL. This reminded me though that there was some hidden logic that 0 would be silently treated as 1, and that worker-threads=0 is special.
I cleaned this up by changing parallel to be of type NonZeroU32, and added the documentation about worker-threads=0.
| interpose_method: Some(InterposeMethod::Ptrace), | ||
| runahead: None, | ||
| scheduler_policy: Some(SchedulerPolicy::Steal), | ||
| scheduler_policy: Some(SchedulerPolicy::Host), |
There was a problem hiding this comment.
Is the [default: "host"] cli help string suffix auto-updated with these changes?
| // By default use 1 worker per host. | ||
| config.hosts.len().try_into().unwrap() |
There was a problem hiding this comment.
Much simpler than I had imagined :)
Parallelism is the right term for how many threads will run simultaneously, *not* concurrency. Expose this as a general option, in preparation for making the concrete number of worker threads to actually create an experimental option.
The exact number of threads to use is now an implementation detail. What end users generally care about is how many to run in parallel, which is now controlled through the `--parallelism` flag. This also changes the default number of workers to be the number of Hosts.
max-concurrencyis now the General optionparallelism.workersis now the Experimental optionworker-threads, and no longer has the-wshortcut. Users should useparallelisminstead. If not set explicitly, the number of worker threads will be set to the number of hosts in the simulation.