Land the per-call publish ordering on master (re-target of #10) - #11
Merged
Conversation
Traits::concurrent_reads is a per-TYPE decision, and a costly one: it makes
every access to the vtable pointer atomic, including the invoke path, which
blocks the scalar replacement a non-escaping callable would otherwise get.
A type used single-threaded in a thousand places, one of which needs the
publish-once pattern, should not have to pay that everywhere.
So the ordering can now also be requested per call, for any Traits:
f.assign( target, std::memory_order_release ); // arm
f.empty ( std::memory_order_acquire ); // probe
`publish_order` (an optional<memory_order>) threads down to the single
publishing store; unset - the default on every existing path - keeps the
Traits-driven behaviour, so nothing changes for callers that do not ask.
The ordered accessors themselves are unconditional now: asking for an
ordering IS the opt-in. What the trait still adds is that the type's own
internal accesses are ordered too, so the guarantee holds by construction
instead of by caller discipline - it remains the recommendation, and the
per-call form carries std::atomic_ref's own contract (while an ordered
access is in flight, every conflicting access must also be ordered).
Because the atomic accessors const_cast the member's qualification away
anyway - std::atomic_ref binds to neither cv nor __restrict, and both
concern the pointee rather than the pointer's own storage - the member
itself needs no conditional type and no `mutable`: it goes back to being
declared exactly as it always was, which also restores the vertical
alignment #9 disturbed. Non-opted-in codegen re-verified byte-identical to
master for a default_traits TU at -O3 -DNDEBUG.
The vtable accessors now speak in references. That separates two things the
pointer form conflated: `vtable_slot()` reads what the pointer HOLDS, which
mid-construction is legitimately not a vtable at all (debug_clear's
invalid_ptr) and is what the identity/validity assertions want; while
`load_vtable()` / `store_vtable()` deal in an actual vtable, so they cannot
be handed null.
The ordered assign ARMS an empty callable, once. That is the mechanism's own
restriction, not an implementation limit: a release store publishes the
writes preceding it, so re-arming would publish over a target a prober may
have observed and be invoking, and the reassignment destroys it under that
reader. Closing that needs deferred reclamation, not ordering. It is
therefore routed through the `direct` path, whose publication is exactly one
store and whose precondition is this same emptiness; the ordinary route's
may-throw case publishes through swap(), i.e. several plain stores, one of
which hands a prober an engaged vtable with nothing ordering the target
behind it (ThreadSanitizer confirms).
For the same reason there is no ordered clear(): disengagement has no
preceding writes to publish, so an ordered clear would order nothing while
reading as though it made disarming safe. The Linux kernel reached the same
conclusion for the identical shape - rcu_assign_pointer() degrades to a
plain WRITE_ONCE() when publishing NULL - and keeps removal on grace periods
rather than pretending a store ordering can retire an object.
Verified clang-22 and gcc-16, Debug and Release, ctest green; the
publish-once test on non-opted-in Traits is ThreadSanitizer-clean over
repeated runs and reports races when the ordered store is removed.
Per-call publish ordering: assign( f, std::memory_order )
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.
Bookkeeping fix, no new code — the diff is exactly #10, already reviewed and merged there.
#10 was stacked on
feat/lock-free-empty-checkand merged into that branch after the branch had itself merged via #9. Both PRs read MERGED and nothing went red, butmasteronly ever received #9's commit:origin/masterisb8cc457 Merge pull request #9, while the per-call commit230b362lives solely on this branch.Merging this brings that commit across. Contents, for reference:
assign( f, std::memory_order )— ordering requestable per CALL for anyTraits, sinceconcurrent_readsis per-TYPE and would tax every use of the type;clear()— disengagement has no preceding writes to publish (same conclusion asrcu_assign_pointer()degrading toWRITE_ONCE()for NULL);masterhas it (theconst_castin the atomic accessors removes the need for a conditional type ormutable), with non-opted-in codegen verified byte-identical;vtable_slot()(what the slot holds, possiblyinvalid_ptrmid-construction) fromload_vtable()/store_vtable().Verification is unchanged from #10: clang-22 and gcc-16, Debug and Release, ctest green; ThreadSanitizer-clean over repeated runs, and it reports races when the ordered store is removed.