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

Added support for negative nthreads and resp. documentation. #4535

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

Conversation

virtualritz
Copy link

Description

This makes nthreads accept negative numbers to mean m_pool->size() + nthreads. I.e. on a 16 core machine specifying nthreads as -2 would make OIIO use 14 cores.
If m_pool->size() < -nthreads the result is clamped at 1. I.e. a single thread would be used.

On that note: the original resolve() does not have a check for the case where m_maxthreads > m_pool->size() and should possibly be clamped to the latter, if so. I kept it that way. Is that intended/a feature or an oversight?

Tests

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • If I added or modified a C++ API call, I have also amended the
    corresponding Python bindings (and if altering ImageBufAlgo functions, also
    exposed the new functionality as oiiotool options).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format before submitting, I definitely will look at the CI
    test that runs clang-format and fix anything that it highlights as being
    nonconforming.

@ssh4net
Copy link
Contributor

ssh4net commented Nov 17, 2024

Why this can be better than

std::thread::hardware_concurrency() - 2
?

@@ -680,14 +688,14 @@ bool OIIO_API resize(ImageBuf &dst, const ImageBuf &src, KWArgs options = {},
/// corresponding portion of `src` (mapping such that the "full" image
/// window of each correspond to each other, regardless of resolution). If
/// `dst` is not yet initialized, it will be sized according to `roi`.
///
///
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please remove all spurious formatting changes that are not directly related to the PR? Thanks.

Copy link
Author

@virtualritz virtualritz Nov 24, 2024

Choose a reason for hiding this comment

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

Yeah, sorry about that. It seems removing trailing white space on save is a default feature in modern IDEs.
I'll get to it sometime next week.
That said: wouldn't it make sense to remove trailing white space in the entire code base to save anyone from going though this on a PR?
I guess I'm asking if you'd consider a separate PR for just that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, certainly. I just want code changes to not have formatting changes to any code that's not changing. A PR that is nothing but formatting changes is fine as long as no code changes. (Among other things, that allows us to add the formatting-only commit hashes to .git-blame-ignore-revs so that they don't change the apparent history or authorship of any actual code.)

@lgritz
Copy link
Collaborator

lgritz commented Nov 17, 2024

Why this can be better than

std::thread::hardware_concurrency() - 2 ?

  1. Because it means 2 less than the pool size, not the HW threading size.
  2. Because it can get passed from all sorts of places where it's not convenient to call C++ std::thread_hardware_concurrency(). Like Python? Or from command line args?

Comment on lines +146 to +152
Specifying a negative number for `nthreads` will reserve `-nthreads`
threads. In this case the actual number of threads used is the number of
cores + `nthreads` + 1. One thread is always reserved for the caller.
For example, on a 32 core system specifying `nthreads` as -2 means the
OpenImageIO will use 31 cores: 32 + `nthreads` + 1. If `-nthreads` is
specified larger than the number of available cores only a single core will
be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify, because I myself was not sure: Does -2 mean "num_hardware_cores - 2" or does it mean "thread_pool_size - 2"?

Comment on lines +602 to +603
if (m_maxthreads < 0)
m_maxthreads = fmax(m_pool->size() + m_maxthreads + 1, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to do "pool_size - n"

@lgritz
Copy link
Collaborator

lgritz commented Nov 25, 2024

I think that your changes only alter the meaning of the nthreads parameter to IBA functions. It is worth noting that there are a few other places where number of threads are passed:

  • The setting of the thread pool size with OIIO::attribute("threads", n)
  • ImageInput/ImageOutput::threads()
  • ImageBuf::threads()
  • Utility functions such as parallel_convert_image()
  • The paropt structure used by parallel loops and such in parallel.h

Do we want to change those also? If we don't, will this create confusion?

Do those other places misbehave if passed negative thread counts? That's something we may never have considered before because... why would anybody do that? But now, we're giving people a reason to pass negative thread counts, at least in some places, which means we should expect people to do it inadvertently it in any of the other places.

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