[PATCH v2] libstdc++: Improve std::lock algorithm

[Matthias Kretz]

On Dienstag, 22. Juni 2021 17:20:41 CEST Jonathan Wakely wrote:
> On Tue, 22 Jun 2021 at 14:21, Matthias Kretz wrote:
> > This does a try_lock on all lockabes even if any of them fails. I think
> > that's
> > not only more expensive but also non-conforming. I think you need to defer
> > locking and then loop from beginning to end to break the loop on the first
> > unsuccessful try_lock.
> 
> Oops, good point. I'll add a test for that too. Here's the fixed code:
> 
>     template<typename _L0, typename... _Lockables>
>       inline int
>       __try_lock_impl(_L0& __l0, _Lockables&... __lockables)
>       {
> #if __cplusplus >= 201703L
>         if constexpr ((is_same_v<_L0, _Lockables> && ...))
>           {
>             constexpr int _Np = 1 + sizeof...(_Lockables);
>             unique_lock<_L0> __locks[_Np] = {
>                 {__l0, defer_lock}, {__lockables, defer_lock}...
>             };
>             for (int __i = 0; __i < _Np; ++__i)

I thought coding style requires a { here?

>               if (!__locks[__i].try_lock())
>                 {
>                   const int __failed = __i;
>                   while (__i--)
>                     __locks[__i].unlock();
>                   return __i;

You meant `return __failed`?

>                 }
>             for (auto& __l : __locks)
>               __l.release();
>             return -1;
>           }
>         else
> #endif
> 
> > [...]
> > Yes, if only we had a wrapping integer type that wraps at an arbitrary N.
> > Like
> > 
> > unsigned int but with parameter, like:
> >   for (__wrapping_uint<_Np> __k = __idx; __k != __first; --__k)
> >   
> >     __locks[__k - 1].unlock();
> > 
> > This is the loop I wanted to write, except --__k is simpler to write and
> > __k -
> > 1 would also wrap around to _Np - 1 for __k == 0. But if this is the only
> > place it's not important enough to abstract.
> 
> We might be able to use __wrapping_uint in std::seed_seq::generate too, and
> maybe some other places in <random>. But we can add that later if we decide
> it's worth it.

OK.

> > I also considered moving it down here. Makes sense unless you want to call
> > __detail::__lock_impl from other functions. And if we want to make it work
> > for
> > pre-C++11 we could do
> > 
> >   using __homogeneous
> >   
> >     = __and_<is_same<_L1, _L2>, is_same<_L1, _L3>...>;
> >   
> >   int __i = 0;
> >   __detail::__lock_impl(__homogeneous(), __i, 0, __l1, __l2, __l3...);
> 
> We don't need tag dispatching, we could just do:
> 
> if _GLIBCXX17_CONSTEXPR (homogeneous::value)
>  ...
> else
>  ...
> 
> because both branches are valid for the homogeneous case, i.e. we aren't
> using if-constexpr to avoid invalid instantiations.

But for the inhomogeneous case the homogeneous code is invalid (initialization 
of C-array of unique_lock<_L1>).

> But given that the default -std option is gnu++17 now, I'm OK with the
> iterative version only being used for C++17.

Fair enough.

-- 
──────────────────────────────────────────────────────────────────────────
 Dr. Matthias Kretz                           https://mattkretz.github.io
 GSI Helmholtz Centre for Heavy Ion Research               https://gsi.de
 std::experimental::simd              https://github.com/VcDevel/std-simd
──────────────────────────────────────────────────────────────────────────