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

Feature flag to set num-threads to host threads #641

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jerrymarino
Copy link
Contributor

Currently, num-threads is hardcoded to 12. This PR sets the value to the
number of cores return by sysctl, or "host threads". This may improve
vertical scaling in some situations for larger core machines.

@google-cla google-cla bot added the cla: yes label Jun 9, 2021
Currently, num-threads is hardcoded to 12. This PR sets the value to the
number of cores return by sysctl, or "host threads". This may improve
vertical scaling in some situations for larger core machines.
@jerrymarino jerrymarino force-pushed the jmarino/use_host_num_threads_rebased branch from ec5fdc6 to 6948f6e Compare June 9, 2021 23:09
Copy link
Member

@thii thii left a comment

Choose a reason for hiding this comment

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

This is nice, presuming there's no side-effects on reproducibility.

@@ -318,6 +319,14 @@ bool SwiftRunner::ProcessArgument(
++itr;
new_arg = output_file_map_path_;
changed = true;
} else if (use_host_num_threads_ && arg == "-num-threads") {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be an error or something instead? like you shouldn't pass this and enable this feature at the same time I don't think?

@@ -102,7 +103,7 @@ static bool StripPrefix(const std::string &prefix, std::string &str) {

SwiftRunner::SwiftRunner(const std::vector<std::string> &args,
bool force_response_file)
: force_response_file_(force_response_file) {
: force_response_file_(force_response_file), use_host_num_threads_(false) {
Copy link
Member

Choose a reason for hiding this comment

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

since the other feature bools aren't here you shouldn't need this one here either i don't think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants