[NativeAOT] Use POSIX primitives for GC bridge processing#12141
[NativeAOT] Use POSIX primitives for GC bridge processing#12141simonrozsival wants to merge 6 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
Clarify that JavaMarshal permits only one outstanding bridge round, so shared_args is intentionally a single-slot handoff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code Review — ✅ LGTM (with minor suggestions)
Verdict: ✅ LGTM · Issues: 0 ❌ · 0
This is a clean, well-scoped refactor that swaps the GC bridge's std::thread / std::binary_semaphore / std::atomic primitives for POSIX pthread + sem_t + compiler __atomic builtins, as part of the libc++-removal effort (#12139).
What was verified
- Correctness of the handoff:
publish_shared_args(RELEASE store +sem_post) /wait_for_shared_args(sem_wait+ ACQUIRE load) preserve the acquire/release ordering of the originalstd::atomic. The single-slot design is safe given the documented JavaMarshal "one outstanding round" invariant. EINTRhandling:sem_waitis correctly retried in a loop onEINTR(native-rules Postmortem#22). 👍- Consistency:
pthreadand__atomic_*builtins are already established conventions in this codebase (assembly-store.cc,os-bridge.cc,pinvoke-override), so the approach fits in well. - Error handling: pthread/semaphore failures are surfaced through the existing
abort_unlesshelpers, consistent with the rest of the host. - Include cleanup: removed
<atomic>,<thread>,<semaphore>,<shared_mutex>,<unordered_map>— none appear to be used elsewhere in the header. 👍 __atomic_*are compiler builtins (not user identifiers), so the reserved-double-underscore rule doesn't apply here.
Suggestions (non-blocking, posted inline)
- 💡 Match
assembly-store.cc's single-step detached-thread creation and consider naming the worker thread for diagnostics. - 💡 A brief note that the semaphore (not the atomics) is the real synchronization point would aid future readers.
Nice, tightly-focused change with a helpful invariant comment. CI shows no required checks failing at review time.
Generated by Android PR Reviewer for #12141 · 84.2 AIC · ⌖ 13.1 AIC · ⊞ 6.8K
Comment /review to run again
| void GCBridge::start_bridge_processing_thread () noexcept | ||
| { | ||
| pthread_t thread {}; | ||
| int ret = pthread_create (&thread, nullptr, bridge_processing_thread_entry, nullptr); |
There was a problem hiding this comment.
🤖 💡 Native C++ — Consider creating the worker already-detached via pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED) before pthread_create, matching the sibling pattern in assembly-store.cc (start_writer_locked). The current create-then-pthread_detach sequence works, but a single-step detached creation is consistent with the rest of the CLR host and avoids a separate failure/abort path.
While here, pthread_setname_np (thread, "gc-bridge") would make this worker identifiable in logcat/tombstones and traces — helpful given it runs the GC bridge on every collection.
(Rule: Consistency with repo native patterns)
| { | ||
| __atomic_store_n (&shared_args, args, __ATOMIC_RELEASE); | ||
|
|
||
| int ret = sem_post (&shared_args_semaphore); |
There was a problem hiding this comment.
🤖 💡 Native C++ — sem_post/sem_wait already establish a happens-before relationship, so the explicit __ATOMIC_RELEASE/__ATOMIC_ACQUIRE on shared_args are technically redundant for this single-slot handoff (which the header comment nicely documents). Keeping them as belt-and-suspenders is fine, but a one-line note here that the semaphore is the real synchronization point — and the atomics just make the intent explicit — would prevent a future reader from assuming lock-free concurrent access is intended.
(Rule: Documentation of concurrency intent)
Summary
Replace the NativeAOT-reachable GC bridge's C++ threading primitives with POSIX primitives.
This removes the
std::thread,std::binary_semaphore, and associated libc++ atomic-wait/thread runtime dependency from this subsystem as part of #12139.Changes
std::threadworker withpthread_create()/pthread_detach();std::binary_semaphorewithsem_t;sem_wait()afterEINTR;shared_argsan intentional single-slot handoff rather than a queue.The bridge-processing algorithm and callback lifecycle are unchanged.
Validation
git diff --check;src/native/native-nativeaot.csproj;src/native/native-clr.csproj;12bc25740.Part of #12139.