Jonathan Wakely writes:
> On 09/05/20 17:01 -0700, Thomas Rodgers via Libstdc++ wrote: <snip> >>+#include <iostream> > > <iostream> shouldn't be here (it adds runtime cost, as well as > compile-time). > Oversight, not removed after debugging it. <snip> > > Can't this just be __old instead of *std::__addressof(__old) ? > Copypasta from elsewhere in the same class, I believe. I'll change it. <snip> > > Isn't alignas(64) already implied by the first data member? > Yes >>+ { >>+ int32_t alignas(64) _M_ver = 0; >>+ int32_t alignas(64) _M_wait = 0; >>+ >>+ // TODO make this used only where we don't have futexes > > Don't we always need these even with futexes, for the types that don't > use a futex? > If we have futexes, we can use the address of _M_ver to wake _M_do_wait() instead of using a condvar for types that don't use a futex directly. >>+ using __lock_t = std::unique_lock<std::mutex>; >+ mutable __lock_t::mutex_type _M_mtx; >>+ >>+#ifdef __GTHREAD_COND_INIT >>+ mutable __gthread_cond_t _M_cv = __GTHREAD_COND_INIT; >>+ __waiters() noexcept = default; > > If we moved std::condition_variable into its own header (or > <bits/std_mutex.h>, could we reuse that here instead of using > __gthread_cond_t directly? > Yes, I started down that route initially, I could revisit it in a future patch as part of also making it's use only necessary when the platform doesn't support futex. >>+ __atomic_notify(const _Tp* __addr, bool __all) noexcept >>+ { >>+ using namespace __detail; >>+ auto& __w = __waiters::_S_for((void*)__addr); >>+ if (!__w._M_waiting()) > > When __platform_wait_uses_type<_Tp> is true, will __w._M_waiting() > ever be true? Won't this always return before notifying? > > Is there meant to be a __waiter constructed here? > __waiter (an RAII type) is constructed in the __atomic_wait(), that increments the _M_wait count on the way into the wait, and decrements it on the way out, __atomic_notify checks to see if that count is non-zero before invoking the platform/semaphore notify because it is cheaper to do the atomic load than it is to make the syscall() when there are no waiters. >>+ return; >>+ >>+ if constexpr (__platform_wait_uses_type<_Tp>::__value) >>+ { >>+ __platform_notify((__platform_wait_t*)(void*) __addr, __all); >>+ } <snip> >>+ struct __platform_semaphore >>+ { >>+ using __clock_t = chrono::system_clock; >>+ >>+ __platform_semaphore(ptrdiff_t __count) noexcept > > Should this constructor be explicit? > Yes. >>+ template<typename _Duration> >>+ _GLIBCXX_ALWAYS_INLINE bool > > Do we really need this to be always_inline? > Probably not, copypasta from elsewhere in the same file. >>+ __try_acquire_until_impl(const chrono::time_point<__clock_t>& __atime) >>noexcept >>+ { >>+ auto __s = chrono::time_point_cast<chrono::seconds>(__atime); >>+ auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s); <snip> >>+ template<typename _Tp> >>+ struct __atomic_semaphore >>+ { >>+ static constexpr size_t _S_alignment = __alignof__(_Tp); >>+ >>+ __atomic_semaphore(_Tp __count) > > Should this be explicit? > Yes. >>+ private: >>+ alignas(_S_alignment) _Tp _M_a; > > Could this just use alignas(__alignof__(_Tp)) _Tp here? There's no > need for the _S_alignment constant if it's only used in one place. > Yes. >>+ }; >>+ >>+#ifdef _GLIBCXX_REQUIRE_POSIX_SEMAPHORE >>+ template<ptrdiff_t __least_max_t> > > Rename __least_max_t here too. > >>+ using __semaphore_base = __platform_semaphore<__least_max_t>; >>+#else >>+# ifdef _GLIBCXX_HAVE_LINUX_FUTEX >>+ template<ptrdiff_t __least_max_t> >>+ using __semaphore_base = std::conditional<(__least_max_t > 0 > > This should use conditional_t<> not conditional<>::type. > > The least-max_value can't be negative. If it's zero, can't we use a > futex or semaphore? So the '__least_max_t > 0' condition is wrong? > Yes. >>+ && __least_max_t < >>std::numeric_limits<__detail::__platform_wait_t>::max()), > > Should that be <= rather than < ? > Likely. >>+ >>__atomic_semaphore<__detail::__platform_wait_t>, >>+ >>__atomic_semaphore<ptrdiff_t>>::type; >>+ // __platform_semaphore >>+# else <snip...>