From tbb::atomic to std::atomic #1908
Conversation
| @@ -286,22 +286,36 @@ class UsdImaging_ResolvedAttributeCache | |||
| // non-time varying data, entries may exist in the cache with invalid | |||
| // values. The version is used to determine validity. | |||
| struct _Entry { | |||
| _Entry() | |||
| _Entry() noexcept | |||
There was a problem hiding this comment.
Could you comment on the addition of the noexcept keyword here? I'm not sure I'm spotting what necessitates the change?
There was a problem hiding this comment.
Oh right I think it'll still compile without it and we don't necessarily need it here, it is to imply the move will always be a move and not a copy. Maybe just have it for move and not the regular constructor?
|
Filed as internal issue #USD-7438 |
|
Hi @boberfly! Thanks for the PR, it's very much appreciated! As noted in the issue thread by @e4lam (#1471 (comment)) , it makes sense to add In order to facilitate merging this work, could we ask that it be broken up such that individual modules can be independently regression and performance tested at our end, and merged one by one? The set of commits that would enable that would be the affected modules and their pch.h files, and then lastly the remainder of the pch.h changes. (1) pcp Apologies for the inconvenience, and thank you for the work! |
|
@meshula no worries I'll get to this soon |
|
@boberfly, any updates on this? Thank you! |
| unsigned cv = _cacheVersion.load(); | ||
| if (v < _cacheVersion | ||
| && entry->version.compare_and_swap(_cacheVersion, v) == v) | ||
| && entry->version.compare_exchange_strong(cv, v)) |
There was a problem hiding this comment.
The argument order here is wrong and leads to a deadlock in the tests. This works:
&& entry->version.compare_exchange_strong(v, _cacheVersion.load())
There was a problem hiding this comment.
Thank you so much, @brechtvl - I see it in the smaller PR.
|
Am I correct in thinking that this PR has now been superseded by the other PR's and we should close this one out? |
|
Yes, I think this one can be closed. |
|
Closing in favor of more targeted PR's. |
Description of Change(s)
Swaps out tbb::atomic with std::atomic, with the help of the people in #1471
_PrototypeTask and _Entry structs need defined copy/move constructors to be able to compile (std::atomic has copy deleted and move undefined). The adding into a TfHashMap with a make_pair is considered a move.
Fixes Issue(s)