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

Allow automatically determining the number of threads #1039

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

oscargus
Copy link
Contributor

Pass -p0 to use all cores (determined by multiprocessing.cpu_count().

@oscargus oscargus force-pushed the autoparallel branch 2 times, most recently from d335ece to ef63b22 Compare July 11, 2024 11:19
@LarsAsplund
Copy link
Collaborator

LarsAsplund commented Jul 11, 2024

Sounds like a good idea but I would like it to take hyper threading into account. I typically run 12 threads on my 6 cores. From what I understand there is also os.cpu_count that might work differently. I'm on my phone so I can't test now. I've also seen (discussion) threads indicating it doesn't always work. Differences between Windows and Linux.

@oscargus
Copy link
Contributor Author

I am quite sure that it does that. I've tested a few computers to confirm.

On my laptop (i7 1260P, 4P + 8E): cpu_count() = 16
On my workstation (some Xeon, 6 cores, 12 threads): cpu_count() = 12
On my compute server (Two 20-core Xeon, 80 threads): cpu_count() = 80

I was a bit unsure what to write in the note as the concept of threads from a software perspective is a bit different from the hardware perspective... Maybe one should write "logical CPUs"? (os.cpu_count() returns the same btw.)

@LarsAsplund
Copy link
Collaborator

Ok, then it should be fine. Regarding the use of "threads". I think we should keep it when explaining the -p option since the verbose option name is --num-threads.

I will look into the PR a bit more when I'm back at my computer but I can't see any major issues with it. I'm not sure how common it is that cpu_count fails to determine the number of threads but I think we should test for that case and raise a descriptive error if it happens.

@LarsAsplund
Copy link
Collaborator

I tested this locally and it works as expected.

I take back my comment on how the -p option is described. I'm good with how it is stated and will merge this PR.

@LarsAsplund LarsAsplund merged commit d13d4a5 into VUnit:master Jul 15, 2024
14 checks passed
@oscargus
Copy link
Contributor Author

I take back my comment on how the -p option is described. I'm good with how it is stated and will merge this PR.

I reformulated it based on your comment, so glad it improved it. :-)

@oscargus oscargus deleted the autoparallel branch July 16, 2024 08:12
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.

2 participants