PROTOTYPE WIP: Add thread-safe read functions to input files #1280
+130
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
lgritz
force-pushed
the
lg-threadsafe
branch
from
bcb37a1
to
999d37a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Flesh this out for ALL the classes and relevant methods ASAP in time for 3.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.
Just hold off on all of it until 3.3.