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

Add thread control to exrmetrics #1904

Conversation

kdt3rd
Copy link
Contributor

@kdt3rd kdt3rd commented Nov 2, 2024

Add ability to test threading modes with exrmetrics

@kdt3rd kdt3rd requested a review from peterhillman November 2, 2024 00:56
Copy link
Contributor

@peterhillman peterhillman left a comment

Choose a reason for hiding this comment

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

This looks good but I wonder whether the new defaults are correct, or at least need better documentation.

Previously, exrmetrics didn't call setGlobalThreadCount so processing is single threaded, right? Now, it multithreads by default, with "a sensible number of threads" (also what you get with -t 0)

Timing statistics from multithreading are more susceptible to variation than when running on one thread, which could be misleading. Would it be better to leave the default as one thread? Adding a short note to the usage message to warn of variation when running multithreaded, and also documenting -t 0, could help.

I suppose a future version could report CPU time as well as elapsed time, so give more stable data and also indications of threading efficiency for each step of the process.

@kdt3rd
Copy link
Contributor Author

kdt3rd commented Nov 2, 2024

yes, -t 0 will be equivalent to -s, which is the same as if you don't setGlobalThreadCount at all - it defaults to 0 which is how I noticed this missing bit in the first place.

can change the default back, as the print should probably print threads and time if it's going to change to multi-threaded by default

@kdt3rd kdt3rd force-pushed the add_thread_control_to_exrmetrics branch from ba3862b to bcec594 Compare November 2, 2024 03:50
Copy link
Contributor

@peterhillman peterhillman 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!

@@ -41,6 +44,9 @@ usageMessage (ostream& stream, const char* program_name, bool verbose = false)
" -p n part number to copy (only one part will be written to output file)\n"
" default is part 0\n"
"\n"
" -m set to multi-threaded (system selected thread count)\n"
" -t n use n threads for processing files\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding default is single threaded just to document behavior?

Add ability to test threading modes with exrmetrics

Signed-off-by: Kimball Thurston <[email protected]>
@kdt3rd kdt3rd force-pushed the add_thread_control_to_exrmetrics branch from bcec594 to d5bcbc8 Compare November 2, 2024 04:57
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM

@cary-ilm cary-ilm merged commit 5ce329c into AcademySoftwareFoundation:main Nov 3, 2024
31 checks passed
cary-ilm pushed a commit that referenced this pull request Nov 3, 2024
Add ability to test threading modes with exrmetrics

Signed-off-by: Kimball Thurston <[email protected]>
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