fix(concurrency): atomic UNIV::TIME / DEVICE_TIME / m_libsLoaded#421
Merged
fix(concurrency): atomic UNIV::TIME / DEVICE_TIME / m_libsLoaded#421
Conversation
TSan flagged three data races on the cpp-modernisation-followup branch (all pre-existing, not introduced by that PR): - UNIV::TIME (plain volatile uint64_t) is written by the scheduler thread in TaskScheduler::timeSlice and read from every other thread; the volatile keyword prevents compiler reordering but does nothing for atomicity across threads. - UNIV::DEVICE_TIME has the same shape, written by the audio callback and read from other threads. - SchemeProcess::m_libsLoaded is a bool set once by the task thread after runtime libs finish loading and spin-waited on by the server thread in serverImpl(). Promote all three to std::atomic. std::atomic<uint64_t> is lock-free on every tier-1 platform (added a static_assert) and has the same size/alignment as uint64_t, so the xtlang JIT's plain `load i64, i64* @TIME` in runtime/bitcode.ll remains layout-compatible. A couple of callers had to change: - AudioDevice.cpp's `UNIV::TIME = UNIV::DEVICE_TIME` is now explicit load/store to avoid the deleted atomic copy assignment. - Three `auto time(UNIV::TIME)` sites become `uint64_t time(UNIV::TIME)` for the same reason (auto would deduce atomic, which isn't copyable). Re-ran the full libs-core + cpp-unit + compiler-unit suite under TSan: the four previously-reported unique races are down to one (the SchemeTask deque iterator race, which belongs with TASK-042).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Follow-up to #420's TSan run. Three of the four reported races had obvious fixes:
UNIV::TIME— plainvolatile uint64_t, written by the scheduler, read from every other thread →std::atomic<uint64_t>.UNIV::DEVICE_TIME— same shape, written by the audio callback →std::atomic<uint64_t>.SchemeProcess::m_libsLoaded— spin-waited bool set once at startup →std::atomic<bool>.std::atomic<uint64_t>is lock-free on every tier-1 platform (added astatic_assert) and has the same size/alignment asuint64_t, so the xtlang JIT's@TIME = external global i64+load i64inruntime/bitcode.llstays layout-compatible. A handful of callers were tweaked for atomic's deleted copy assignment:UNIV::TIME = UNIV::DEVICE_TIME→ explicit.store(…load()).auto time(UNIV::TIME)→uint64_t time(UNIV::TIME)(auto would deduce atomic, which isn't copyable).Verification
Re-ran
libs-core | cpp-unit | compiler-unitunderEXTEMPORE_SANITIZE=tsan:The remaining race is the SchemeTask deque iterator one that belongs to TASK-042 (audio-thread dispatcher races) — left as-is here.
Test plan
SchemeS7.cpp:309(TIME),SchemeS7.cpp:79(TIME),SchemeProcess.cpp:293(m_libsLoaded) all clean