Resolve data races found by ThreadSanitizer #121
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
|
|
QuantamHD
reviewed
Nov 7, 2024
graph/Graph.cc
Outdated
Comment on lines
1369
to
1382
| unsigned char expected = bfs_in_queue_; | ||
| unsigned char desired; | ||
| do { | ||
| if ((value && IN_QUEUE(expected, index)) || (!value && !IN_QUEUE(expected, index))) { | ||
| return false; | ||
| } | ||
| desired = expected; | ||
| SET_IN_QUEUE(value ? desired : expected, index); | ||
| CLEAR_IN_QUEUE(value ? expected : desired, index); | ||
| } while (!bfs_in_queue_.compare_exchange_weak(expected, desired)); | ||
| return true; |
There was a problem hiding this comment.
How does this compare to just locking. I wonder if the additional memory locality and smaller memory footprint make up for the coarser lock
There was a problem hiding this comment.
Locking is slightly slower - a few benchmarks show slowdown between 0.5% and 2%
eszpotanski
force-pushed
the
eszpot/thread-safety
branch
2 times, most recently
from
30209e0 to
1a3f0b7
Compare
|
@jjcherry56 Do these changes seem reasonable, or is there a better way to address these data races? The atomic bitset seems a bit complicated to me, but it might be worth the speed up. |
Signed-off-by: Eryk Szpotanski <[email protected]> Co-authored-by: Krzysztof Bieganski <[email protected]>
eszpotanski
force-pushed
the
eszpot/thread-safety
branch
from
1a3f0b7 to
4ccc9d8
Compare
|
As b4be3c5 fixed part of the data races, I've updated this PR to contain fixes for remaining ones - caused by:
|
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.
This PR fixes following data races found by ThreadSanitizer when OpenSTA is run by OpenROAD-flow-scripts:
These changes incur a significant slowdown (between 10% and 40%, depending on number of threads, used design or stage).
Note that these are only the races found by ThreadSanitizer, not necessarily all of them.