[ORC] Fix synchronization in CoreAPIsTest. #144556
Conversation
The code previously appeared to have a (benign?) race condition on `WorkThreads.size`, since it was being accessed outside of the mutex lock that protected it on the threads. This is usually okay since 1a1d6e6, but doesn't seem reliable in general, so fix this code to express the intent more accurately. This relies on the same general principles as ref-counting, where each existing reference (thread) can add new references (threads) because they already have a reference themselves (until joined).
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| std::vector<std::thread> WorkThreads; | ||
| SmallVector<std::thread,0> WorkThreads; |
There was a problem hiding this comment.
Is this change in vector type necessary? If not it'd be better to leave out.
There was a problem hiding this comment.
pop_back_val is only in SmallVector, so it made the code simpler. As far as I know, they are essentially the same type, except the LLVM one has a better API on top, plus it has bounds checking on [] which the std::vector doesn't have for more safety.
| std::lock_guard<std::mutex> Lock(WorkThreadsMutex); | ||
| std::unique_lock Lock(WorkThreadsMutex); |
There was a problem hiding this comment.
I think this could remain a lock_guard, since manual locking/unlocking isn't needed here.
There was a problem hiding this comment.
That is true, but it just seemed more simple to use the same type everywhere: I don't actually know why std::lock_guard exists when std::unique_lock appears to be simply better than it in every way?
|
bump? |
The code previously appeared to have a (benign?) race condition on
WorkThreads.size, since it was being accessed outside of the mutex lock that protected it on the threads. This is usually okay since 1a1d6e6, but doesn't seem reliable in general, so fix this code to express the intent more accurately. This instead relies on the same general principles as ref-counting, where each existing reference (thread) can add new references (threads) because they already have a reference themselves (until joined).