diff --git a/include/psi/functionoid/detail/callable_base.hpp b/include/psi/functionoid/detail/callable_base.hpp index e2ca2f1..b8f908b 100644 --- a/include/psi/functionoid/detail/callable_base.hpp +++ b/include/psi/functionoid/detail/callable_base.hpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -845,7 +846,7 @@ class callable_base : public callable_tag empty_handler_traits::allowsSmallObjectOptimization ); empty_handler_manager::assign( EmptyHandler(), p_function_->functor_, std::allocator() ); - p_function_->p_vtable_ = &empty_handler_vtable_; + p_function_->store_vtable( &empty_handler_vtable_ ); } } @@ -880,7 +881,7 @@ class callable_base : public callable_tag callable_base( no_eh_state_construction_trick_tag, Constructor const constructor, Args && ... args ) noexcept( noexcept( constructor( std::declval(), std::forward( args )... ) ) ) { auto const & vtable( constructor( *this, std::forward( args )... ) ); - BOOST_ASSUME( p_vtable_ == &vtable ); + BOOST_ASSUME( load_vtable( std::memory_order_relaxed ) == &vtable ); } // destructor = trivial promises no target ever needs destroying (and the @@ -896,14 +897,31 @@ class callable_base : public callable_tag protected: bool empty( void const * const p_empty_handler_vtable ) const noexcept { return get_vtable().is_empty_handler_vtable( p_empty_handler_vtable ); } - /// \todo Add atomic vtable accessors that would enable lock-free operation - /// for basic functionality (such as empty(), clear() and operator()()) w/o - /// requiring an additional std::atomic is_my_functionoid_set-like - /// variable. - /// Making the vtable pointer a std::atomic is not an - /// option currently because even with std::memory_order_relaxed access the - /// variable is accessed 'like a volatile' which produces bad codegen (e.g. - /// it is reread from memory for every access). + /// Engagement probe for a reader that may race the thread which assigns to + /// this callable. Only the vtable pointer load is ordered - what it orders + /// is the target buffer written before it (see store_vtable()). + bool empty( void const * const p_empty_handler_vtable, std::memory_order const order ) const noexcept + requires ( Traits::concurrent_reads ) + { + auto const p_vtable{ load_vtable( order ) }; + BOOST_ASSUME( p_vtable ); + return p_vtable->is_empty_handler_vtable( p_empty_handler_vtable ); + } + + //////////////////////////////////////////////////////////////////////////// + // Atomic vtable accessors: the vtable pointer _is_ the engagement state, so + // lock-free empty()/operator bool() need no additional + // std::atomic is_my_functionoid_set-like member (which would also + // cost the genuinely trivial special members and the trivially copyable + // guarantee). Making the member itself a std::atomic is + // still not an option: even memory_order_relaxed access is then compiled + // 'like a volatile' (rereading from memory for every access), and it is not + // movable. std::atomic_ref is the way out - the member stays a plain + // pointer, so every Traits without concurrent_reads keeps the existing + // codegen exactly, and only the opted-in instantiation pays. + /// Both sides must go through these accessors: one plain store racing one + /// atomic load is still a data race, and no amount of atomicity on the + /// reader alone closes it. /// Atomic operations on non-atomic data: /// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4013.html /// Making std::function safe for concurrency: @@ -912,10 +930,30 @@ class callable_base : public callable_tag /// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4058.pdf /// Overloaded and qualified std::function: /// (for 'automatic' atomic vtable access through volatile member function - /// overloads) + /// overloads - rejected: a volatile-qualified overload set duplicates the + /// entire API surface, a single memory_order overload does not) /// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0045r0.pdf /// (08.11.2016.) (Domagoj Saric) - auto const & get_vtable() const noexcept { BOOST_ASSUME( p_vtable_ ); return *p_vtable_; } + vtable const * load_vtable( std::memory_order const order ) const noexcept + { + if constexpr ( Traits::concurrent_reads ) { return std::atomic_ref{ p_vtable_ }.load( order ); } + else { return p_vtable_; } + } + + /// The publishing store. Every assignment path writes the target buffer + /// _first_ and the vtable pointer last, so this release store is exactly + /// the edge a reader's acquire load of it needs: observing a non-empty + /// vtable implies the target it describes is fully constructed. + /// Deliberately not exposed with a memory_order parameter: a weaker + /// publish would defeat the mechanism, so there is no legitimate choice for + /// a caller to make - only a way to get it wrong. + void store_vtable( vtable const * const p_vtable ) noexcept + { + if constexpr ( Traits::concurrent_reads ) { std::atomic_ref{ p_vtable_ }.store( p_vtable, std::memory_order_release ); } + else { p_vtable_ = p_vtable; } + } + + auto const & get_vtable() const noexcept { auto const p_vtable{ load_vtable( std::memory_order_relaxed ) }; BOOST_ASSUME( p_vtable ); return *p_vtable; } buffer & functor() const noexcept { return functor_; } @@ -955,16 +993,16 @@ class callable_base : public callable_tag auto const same_traits{ std::is_convertible_v *, callable_base const *> }; if constexpr ( same_traits ) { - BOOST_ASSUME( &functor_vtable == f.p_vtable_ ); + BOOST_ASSUME( &functor_vtable == f.load_vtable( std::memory_order_relaxed ) ); } if constexpr ( direct ) { BOOST_ASSUME( &f != static_cast( this ) ); BOOST_ASSERT ( - ( this->p_vtable_ == &empty_handler_vtable ) || + ( this->load_vtable( std::memory_order_relaxed ) == &empty_handler_vtable ) || // just being constructed/inside a no_eh_state_construction_trick constructor in a debug build: - ( this->p_vtable_ == invalid_ptr ) + ( this->load_vtable( std::memory_order_relaxed ) == invalid_ptr ) ); assign_functionoid_direct( std::forward( f ), empty_handler_vtable ); } @@ -1004,10 +1042,10 @@ class callable_base : public callable_tag // functionoid.hpp as to why a null vtable is allowed and expected // here. // (02.11.2010.) (Domagoj Saric) - BOOST_ASSERT( this->p_vtable_ == &empty_handler_vtable || /*just being constructed/inside a no_eh_state_construction_trick constructor in a debug build:*/ this->p_vtable_ == invalid_ptr ); + BOOST_ASSERT( this->load_vtable( std::memory_order_relaxed ) == &empty_handler_vtable || /*just being constructed/inside a no_eh_state_construction_trick constructor in a debug build:*/ this->load_vtable( std::memory_order_relaxed ) == invalid_ptr ); using functor_manager = detail::functor_manager, Allocator, buffer>; functor_manager::assign( std::forward( f ), this->functor_, a ); - this->p_vtable_ = &functor_vtable; + this->store_vtable( &functor_vtable ); } else { @@ -1050,7 +1088,7 @@ class callable_base : public callable_tag using functor_manager = functor_manager; this->destroy(); functor_manager::assign( std::forward( f ), this->functor_, a ); - this->p_vtable_ = &functor_vtable; + this->store_vtable( &functor_vtable ); } template @@ -1069,7 +1107,7 @@ class callable_base : public callable_tag using functor_manager = functor_manager, Allocator, buffer>; callable_base tmp( empty_handler_vtable, EmptyHandler() ); functor_manager::assign( std::forward( f ), tmp.functor_, a ); - tmp.p_vtable_ = &functor_vtable; + tmp.store_vtable( &functor_vtable ); this->swap( tmp, empty_handler_vtable ); } @@ -1078,14 +1116,14 @@ class callable_base : public callable_tag { static_assert( Traits::copyable != support_level::na, "Callable not copyable" ); source.get_vtable().clone( source.functor_, this->functor_ ); - p_vtable_ = &source.get_vtable(); + store_vtable( &source.get_vtable() ); } void assign_functionoid_direct( callable_base && source, vtable const & empty_handler_vtable ) noexcept( ( Traits::moveable >= support_level::nofail ) || ( Traits::moveable == support_level::na && Traits::copyable >= support_level::nofail ) ) { source.move_to( *this ); - this ->p_vtable_ = &source.get_vtable(); - source.p_vtable_ = &empty_handler_vtable; + this ->store_vtable( &source.get_vtable() ); + source.store_vtable( &empty_handler_vtable ); } static constexpr bool compatible_vtable_function_entry( support_level const me, support_level const other ) noexcept @@ -1156,7 +1194,7 @@ class callable_base : public callable_tag auto & source_vtable{ source.get_vtable() } ; static_assert( sizeof( *p_vtable_ ) == sizeof( source_vtable ) ); - p_vtable_ = reinterpret_cast( &source_vtable ); + store_vtable( reinterpret_cast( &source_vtable ) ); } template @@ -1194,7 +1232,12 @@ private: template friend class callable_base; class safe_mover_base; template class safe_mover; - vtable const * __restrict p_vtable_; + // __restrict promises the pointer is not aliased - which is exactly what + // concurrent_reads says it is (and std::atomic_ref cannot bind to a + // restrict-qualified lvalue anyway), so the opt-in drops it. + // mutable: load_vtable() is const, and std::atomic_ref's const-T + // specialization is both unnecessary here and libc++-buggy. + mutable std::conditional_t p_vtable_; mutable buffer functor_ ; }; // class callable_base @@ -1220,7 +1263,7 @@ class callable_base::safe_mover_base empty_function_to_move_to_{ empty_function_to_move_to }, empty_handler_vtable_ { empty_function_to_move_to.get_vtable() } { - BOOST_ASSERT( empty_function_to_move_to_.p_vtable_ == &empty_handler_vtable_ ); + BOOST_ASSERT( empty_function_to_move_to_.load_vtable( std::memory_order_relaxed ) == &empty_handler_vtable_ ); move( function_to_guard, empty_function_to_move_to_, empty_handler_vtable_ ); } @@ -1230,8 +1273,8 @@ class callable_base::safe_mover_base static void move( callable_base & source, callable_base & destination, vtable const & empty_handler_vtable ) noexcept { source.move_to( destination ); - destination.p_vtable_ = source.p_vtable_; - source .p_vtable_ = &empty_handler_vtable; + destination.store_vtable( source.load_vtable( std::memory_order_relaxed ) ); + source .store_vtable( &empty_handler_vtable ); } protected: diff --git a/include/psi/functionoid/functionoid.hpp b/include/psi/functionoid/functionoid.hpp index fd7a820..207fa54 100644 --- a/include/psi/functionoid/functionoid.hpp +++ b/include/psi/functionoid/functionoid.hpp @@ -194,6 +194,21 @@ class callable /// Determine if the function is empty (i.e. has an empty target). bool empty() const noexcept { return function_base::empty( &empty_handler_vtable() ); } + /// Same question, asked from a thread that may be racing the one which + /// assigns the target - available only for Traits::concurrent_reads (see + /// policies.hpp for what the opt-in costs and, more importantly, what it + /// does and does not make safe). + /// memory_order_acquire is the ordering that makes the answer usable: + /// a false result then also orders the target itself, so reading it + /// afterwards - invoking it, target()-ing it - is race-free with respect to + /// the assignment that published it. memory_order_relaxed answers the + /// question alone and orders nothing else. + bool empty( std::memory_order const order ) const noexcept + requires ( Traits::concurrent_reads ) + { + return function_base::empty( &empty_handler_vtable(), order ); + } + void swap( callable & other ) noexcept { static_assert( sizeof( callable ) == sizeof( function_base ), "Internal inconsistency" ); diff --git a/include/psi/functionoid/policies.hpp b/include/psi/functionoid/policies.hpp index 110630d..4cc1a79 100644 --- a/include/psi/functionoid/policies.hpp +++ b/include/psi/functionoid/policies.hpp @@ -89,6 +89,42 @@ struct std_traits static constexpr auto rtti = true; static constexpr auto dll_safe_empty_check = true; + /// Opt-in for callables whose engagement is probed - empty() / + /// operator bool() - from a thread that may concurrently race the one + /// assigning to them (the build-once / publish-once pattern: one thread + /// assigns the target, others poll until they observe it). It makes every + /// access to the internal vtable pointer atomic (std::atomic_ref, so the + /// member stays a plain pointer and the type stays trivially copyable when + /// its Traits say so) and adds the empty( std::memory_order ) overload. + /// Off by default, and the cost is larger than "one extra store": an + /// atomic access is not elided even when the compiler can prove the object + /// never escapes the function. Neither clang 22 nor gcc 16 promotes such an + /// object to registers - the atomics pin it to the stack and block the + /// devirtualization that normally follows. Measured, x86-64 -O3 -DNDEBUG, + /// both compilers in agreement: a purely local callable constructed from a + /// lambda and invoked once folds to 2 instructions without the opt-in and + /// to ~40 with it. (The as-if rule would permit the elision - an object no + /// other thread can reference cannot observe the difference - but neither + /// compiler implements it.) + /// For what this is actually for, a shared long-lived callable whose + /// address escapes anyway, there is no such scalar replacement to lose and + /// the cost is only the atomic accesses themselves: ~+15% instructions on + /// an assign-and-publish. Ordinary single-threaded use should still never + /// pay it, hence the opt-in. + /// Note that this orders the vtable pointer only. It publishes the + /// target and makes 'is it engaged' race-free; it does not make assignment + /// itself, or a concurrent invocation of a target being reassigned, safe. + /// What makes the ordered read useful is a property of the *writer*, not + /// of the type: every assignment path here writes the target buffer first + /// and the vtable pointer last, so that one store is a genuine publication + /// point and an acquire load of it orders everything before it. A reader + /// that observes an engaged callable and then invokes it is safe only for + /// as long as no one reassigns or clears it - clear() and a subsequent + /// assign() destroy the target the reader is about to call. This is a + /// publish-once mechanism; it does not make the callable's whole lifetime + /// concurrent. + static constexpr bool concurrent_reads = false; + static constexpr std::uint8_t sbo_size = 4 * sizeof( void * ); static constexpr std::uint8_t sbo_alignment = alignof( std::max_align_t ); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 5220c87..7d9e66c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -12,6 +12,7 @@ add_executable( functionoid_smoke callable_invoke_test.cpp nested_callable_test.cpp vtable_attrs_test.cpp + concurrent_reads_test.cpp ) target_link_libraries( functionoid_smoke PRIVATE GTest::gtest_main Psi::Functionoid ) diff --git a/test/concurrent_reads_test.cpp b/test/concurrent_reads_test.cpp new file mode 100644 index 0000000..99cbcf4 --- /dev/null +++ b/test/concurrent_reads_test.cpp @@ -0,0 +1,153 @@ +// Traits::concurrent_reads: the vtable pointer doubles as the engagement flag, +// so empty()/operator bool() can be answered lock-free - and race-free against +// the thread doing the assigning - without the type growing an atomic member +// (which would also cost it the trivially-copyable guarantee). +// The publish-once/poll-until-engaged shape below is the one this exists for; +// run it under ThreadSanitizer for the part a single-threaded assertion cannot +// check. +#include + +#include + +#include +#include +#include +#include + +namespace { + +namespace pf = psi::functionoid; + +struct concurrent_traits : pf::default_traits +{ + static constexpr bool concurrent_reads = true; +}; + +// Fully trivial: the trivially-copyable guarantee must survive the opt-in. +struct trivial_concurrent_traits : concurrent_traits +{ + static constexpr auto copyable = pf::support_level::trivial; + static constexpr auto moveable = pf::support_level::trivial; + static constexpr auto destructor = pf::support_level::trivial; + static constexpr auto is_noexcept = true; +}; + +// Vtable-compatible with concurrent_traits but not the same Traits: exercises +// the cross-Traits assignment path (assign_functionoid_direct's reinterpreted +// vtable), which is what psi::sweater's generic backend does. +struct other_concurrent_traits : concurrent_traits +{ + static constexpr auto copyable = pf::support_level::nofail; +}; + +using plain_fn = pf::callable; +using concurrent_fn = pf::callable; +using trivial_fn = pf::callable; + +// The overload is opt-in: absent unless the Traits ask for it. +template +concept has_ordered_empty = requires ( Callable const & c ) { c.empty( std::memory_order_acquire ); }; + +static_assert( !has_ordered_empty ); +static_assert( has_ordered_empty ); + +// The whole point of publishing through the vtable pointer rather than an added +// atomic member: no size cost, and no loss of triviality. +static_assert( sizeof( concurrent_fn ) == sizeof( plain_fn ) ); +static_assert( std::is_trivially_copyable_v ); + +} // namespace + +TEST( ConcurrentReads, OrderedEmptyAgreesWithPlainEmpty ) +{ + concurrent_fn f; + EXPECT_TRUE( f.empty( ) ); + EXPECT_TRUE( f.empty( std::memory_order_acquire ) ); + EXPECT_TRUE( f.empty( std::memory_order_relaxed ) ); + + f = [] { return 42; }; + EXPECT_FALSE( f.empty( ) ); + EXPECT_FALSE( f.empty( std::memory_order_acquire ) ); + EXPECT_EQ ( f(), 42 ); + + f.clear(); + EXPECT_TRUE( f.empty( std::memory_order_acquire ) ); +} + +// The cross-Traits assignment path publishes through the same accessor. +TEST( ConcurrentReads, CrossTraitsAssignmentPublishes ) +{ + using other_fn = pf::callable; + + other_fn source{ [] { return 7; } }; + concurrent_fn destination; + ASSERT_TRUE( destination.empty( std::memory_order_acquire ) ); + + destination = source; + EXPECT_FALSE( destination.empty( std::memory_order_acquire ) ); + EXPECT_EQ ( destination(), 7 ); +} + +TEST( ConcurrentReads, OrderedEmptyTracksEveryEngagingOperation ) +{ + concurrent_fn f; + + f.assign( [] { return 1; } ); + EXPECT_FALSE( f.empty( std::memory_order_acquire ) ); + + concurrent_fn const copy{ f }; + EXPECT_FALSE( copy.empty( std::memory_order_acquire ) ); + + concurrent_fn moved{ std::move( f ) }; + EXPECT_FALSE( moved.empty( std::memory_order_acquire ) ); + + concurrent_fn empty_target; + moved.swap( empty_target ); + EXPECT_TRUE ( moved .empty( std::memory_order_acquire ) ); + EXPECT_FALSE( empty_target.empty( std::memory_order_acquire ) ); + + empty_target.assign( nullptr ); + EXPECT_TRUE( empty_target.empty( std::memory_order_acquire ) ); +} + +// Publish-once: one thread assigns, others poll until they observe engagement +// and then invoke. The acquire load is what makes reading the target - which +// the assigning thread wrote before publishing the vtable pointer - not a race. +TEST( ConcurrentReads, PublishOnceIsObservedWithItsTarget ) +{ + static constexpr auto readers{ 4 }; + + for ( auto round{ 0 }; round < 64; ++round ) + { + // Heap-allocated so that the target's own storage is fresh each round + // (a stack slot would be reused and could hide a missing edge). + auto const p_function{ std::make_unique() }; + auto & function { *p_function }; + + std::atomic go{ false }; + std::atomic observed{ 0 }; + + std::vector pollers; + for ( auto reader{ 0 }; reader < readers; ++reader ) + { + pollers.emplace_back( [&] + { + go.wait( false, std::memory_order_acquire ); + while ( function.empty( std::memory_order_acquire ) ) { std::this_thread::yield(); } + observed.fetch_add( function(), std::memory_order_relaxed ); + } ); + } + + std::jthread writer{ [&] + { + go.store( true, std::memory_order_release ); + go.notify_all(); + function = [ round ] { return round; }; + } }; + + pollers.clear(); // join + writer .join (); + + EXPECT_EQ( observed.load( std::memory_order_relaxed ), round * readers ); + } +}