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

feat: rename ShellOtps to GlobalOpts #9313

Merged
merged 23 commits into from
Nov 20, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Nov 13, 2024

Motivation

Related: #8408

Solution

  • Renames ShellOpts to GlobalOpts.
  • Only selectively passes down GlobalOpts where required, this can be the case when wanting to indicate a conflicts_with a global argument. In most cases when checking against the shell configuration it is probably easier to use the e.g. shell::is_json variant over passing it all the way down.

@zerosnacks zerosnacks marked this pull request as ready for review November 13, 2024 16:11
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

impl looks good! there's a regression re the way rayon thread pool is spawned though, a command like

forge test --show-progress --jobs 3

always fail now with Error: The global thread pool has already been initialized.

same on cast create2

cast create2 --starts-with dead --case-sensitive --deployer 0x0000000000FFe8B47B3e2130213B802212439497 --init-code-hash 0x0c591f26891d6443cf08c5be3584c1e6ae10a4c2f07c5c53218741e9755fb9cd --jobs 5

fails with

Error: The global thread pool has already been initialized.

I think we should keep the current behavior, that is if --jobs flag not passed then we use rayon default (the number of CPUs available) instead single thread.

Probably this one should also follow the config

foundry/crates/cast/src/lib.rs

Lines 2062 to 2063 in a65a5b1

let num_threads = std::thread::available_parallelism().map_or(1, |n| n.get());
let found = AtomicBool::new(false);

@zerosnacks
Copy link
Member Author

zerosnacks commented Nov 14, 2024

impl looks good! there's a regression re the way rayon thread pool is spawned though, a command like

Thanks, good catch!

I think we should keep the current behavior, that is if --jobs flag not passed then we use rayon default (the number of CPUs available) instead single thread.

I had implemented it this way to avoid spawning the global thread pool as the vast majority of commands are not multi-threaded, cc @DaniPopes should we spawn by default?

@zerosnacks zerosnacks marked this pull request as draft November 14, 2024 12:05
crates/chisel/bin/main.rs Outdated Show resolved Hide resolved
crates/cast/bin/cmd/create2.rs Outdated Show resolved Hide resolved
crates/cli/src/opts/global.rs Outdated Show resolved Hide resolved
@grandizzy
Copy link
Collaborator

wonder how (if) this would also affect the threadpools spawned in deps (and if we want to or not) like compilers

https://github.com/foundry-rs/compilers/blob/034ecd611eef030217c4b363a794226cd50b4b9e/crates/compilers/src/compile/project.rs#L529-L530

@zerosnacks
Copy link
Member Author

wonder how (if) this would also affect the threadpools spawned in deps (and if we want to or not) like compilers

https://github.com/foundry-rs/compilers/blob/034ecd611eef030217c4b363a794226cd50b4b9e/crates/compilers/src/compile/project.rs#L529-L530

Good point, whenever Rayon is first accessed it will spawn a global thread pool with default settings (unless configured).

Right now it will attempt to spawn for a thread for each job here:

foundry_compilers::project::ProjectCompiler::with_sources(project, sources)?

I'm not sure what we are optimizing for - not running with the default settings of available logical CPUs? It seems desirable, unless specifically required to be limited. A possible motivation would be to pay the cost of spawning upfront.

cc @DaniPopes

@zerosnacks zerosnacks self-assigned this Nov 18, 2024
@zerosnacks zerosnacks added the T-to-discuss Type: requires discussion label Nov 18, 2024
@zerosnacks zerosnacks changed the title feat: introduce global thread pool and rename ShellOtps to GlobalOpts feat: rename ShellOtps to GlobalOpts Nov 20, 2024
@zerosnacks zerosnacks removed the T-to-discuss Type: requires discussion label Nov 20, 2024
@zerosnacks
Copy link
Member Author

zerosnacks commented Nov 20, 2024

Decided to remove the global thread pool for now as it requires further discussion and renamed #8408 as a follow-up to add it

@zerosnacks zerosnacks marked this pull request as ready for review November 20, 2024 13:16
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm!

@DaniPopes DaniPopes merged commit 622f922 into master Nov 20, 2024
21 checks passed
@DaniPopes DaniPopes deleted the zerosnacks/rename-shellotps-globalopts branch November 20, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants