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

PROTOTYPE WIP: Add thread-safe read functions to input files #1280

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

Conversation

lgritz
Copy link
Contributor

@lgritz lgritz commented Sep 29, 2022

The fact that OpenEXR read methods (readPixels, readTiles, etc.) all require a prior call to setFrameBuffer() prevents truly concurrent reads by multiple threads because the calling application needs to maintain a mutex that keeps any other threads from reading from the same file between the two function calls.

This PR is aimed to implement a new set of API calls, variety of the readPixels et al. that take a const FrameBuffer& as parameter rather than relying on saved state, and thus allows truly concurrent reads by multiple threads -- if they use these new safe versions, obviously the old ones continue to be unsafe to use concurrently.

It's a lot of work to do this right! And I'm not aiming to make it good right now, but only to minimally stake out the API and make it functional (even if inefficient) so that we are essentially "reserving" the API just in time for a 3.2 release, and then we can iterate on improving the implementation underneath in subsequent patches, without changing APIs or breaking ABI compatibility within the 3.2 family.

At the moment, I'm just posting a subset of the work as a prototype so people can see where I'm going with it. I've implemented a simple design (just lock internally) for a couple classes. So I'm seeking feedback on which of the following options are preferred:

  1. Flesh this out for ALL the classes and relevant methods ASAP in time for 3.2.

  2. Since these are not virtual methods, adding them doesn't actually break the ABIs, so don't rush this, we can add them (all at once, or incrementally) in subsequent 3.2.x patch releases.

  3. Just hold off on all of it until 3.3.

The fact that OpenEXR read methods (readPixels, readTiles, etc.)  all
require a prior call to setFrameBuffer() prevents truly concurrent
reads by multiple threads because the calling application needs to
maintain a mutex that keeps any other threads from reading from the
same file between the two function calls.

This PR is aimed to implement a new set of API calls, variety of
the readPixels et al. that take a `const FrameBuffer&` as parameter
rather than relying on saved state, and thus allows truly concurrent
reads by multiple threads -- if they use these new safe versions,
obviously the old ones continue to be unsafe to use concurrently.

It's a lot of work to do this right! And I'm not aiming to make it
good right now, but only to minimally stake out the API and make it
functional (even if inefficient) so that we are essentially
"reserving" the API just in time for a 3.2 release, and then we can
iterate on improving the implementation underneath in subsequent
patches, without changing APIs or breaking ABI compatibility within
the 3.2 family.

At the moment, I'm just posting a subset of the work as a prototype
so people can see where I'm going with it. I've implemented a simple
design (just lock internally) for a couple classes. So I'm seeking
feedback on which of the following options are preferred:

1. Flesh this out for ALL the classes and relevant methods ASAP in
   time for 3.2.

2. Since these are not virtual methods, adding them doesn't actually
   break the ABIs, so don't rush this, we can add them (all at once,
   or incrementally) in subsequent 3.2.x patch releases.

3. Just hold off on all of it until 3.3.

Signed-off-by: Larry Gritz <[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.

1 participant