Review: ImageCacheFile refactor #7
Conversation
…lacement of the tile & file mutexes to avoid false sharing (moving them from next to each other to next to the structures they protect).
| // here if more metadata have significance later! | ||
| if (tf->m_swrap == dup->m_swrap && tf->m_twrap == dup->m_twrap && | ||
| tf->m_rwrap == dup->m_rwrap && | ||
| tf->m_datatype == dup->m_datatype && |
There was a problem hiding this comment.
Unrelated to this review - but how can the same file end up with two different datatypes?
There was a problem hiding this comment.
The same file can't have two different data types, but two different files that were both created from the same source texture (and thus have the same fingerprint) could have various differences that would cause you to NOT want to substitute one for the other.
|
LGTM |
|
Because any changes to the hard-won thread safety of IC seems dangerous, I am inclined to merge it into the trunk and leave it there for a while before back-porting to 0.9. Chris, can you accept having a private OIIO trunk that you are building against while you test your field3d work (which is why I made this change now)? We'll get it merged to 0.9 when we need to do a real build for production. By then, between the two of us testing it (and anybody else using the trunk), we should find any problems and be relatively confident about merging. |
…e expect m_validspec to be true, not at this point any more.
…cheFile refactor.
Try to solve once and for all the problem where several ImageCache client threads all simultaneously asking for the same texture that's not yet in cache may each open it separately (and redundantly), with all but the first one eventually closing and discarding.
Modeling on how we handle tiles, now we (1) lock the file cache, add the new file to the file cache, but do not open the file, and unlock the cache; then (2) open the file if it has not yet been opened (locking just that file's m_input_mutex if necessary).
I think that this simplifies the find_file function overall, reduces locking since adding the file to the cache is much less expensive than fully opening it, and ought to eliminate this whole business of redundant opens.
This is very risk and deserves extensive testing as well as smart people thinking hard about the review. Definitely not for porting to 0.8, though if our initial tests are positive I will consider porting to the (not quite formally locked down) 0.9.