BlindDataHolder threading fix #415
Conversation
BlindDataHolder::blindData() was not thread safe for uninitialized m_data because it didn't have a lock. There's an option to add this lock, but I went for always initializing m_data instead, because it makes things simpler and doesn't break binary compatibility.
|
Seems like it was changed to initialise lazily in 58e2c83 - there's no reasoning in the commit message but I'd presume to save a little bit of memory and time in the event that it's not used at all. Dunno if we need to worry about that or not, since there was no mention of the magnitude of the savings in the commit. And yeah, it's Saturday morning - I'll let you draw your own conclusions... |
|
Looks like "- only saved the BlindDataHolder section in the IndexedIO if the CompoundData contains data." is the important bit there. I've just added a unit test for that |
davidsminor
force-pushed
the
blindDataThreadingFix
branch
from
a908d51 to
45aea27
Compare
|
oops - just fixed that commented bit in the test |
test/IECore/BlindDataHolder.py
Outdated
There was a problem hiding this comment.
I think this should just be testing for "test/data/BlindDataHolder" - if the blind data were to be written it'd be into the BlindDataHolder container rather than the Group one. And we should test for the higher level directory, because excluding that gives the biggest storage optimisation.
There was a problem hiding this comment.
ok - just fixed that
davidsminor
force-pushed
the
blindDataThreadingFix
branch
from
45aea27 to
ae84633
Compare
|
Looks good to me. I've just restarted the Travis tests because they went over the time limit. Once they've passed I think we're good to merge. |
BlindDataHolder threading fix
2 participants
BlindDataHolder::blindData() was not thread safe for uninitialized m_data because it didn't have a lock, which was breaking highly instanced renders on one of our shows.
Instead of adding a lock, I went for just always initializing m_data, as it makes things simpler anyway and doesn't break compatibility. What was the reason for initializing it to zero?
And yeah, I am a loser for doing this at 9pm on a Friday...