Re: [PATCH v3] libstdc++: Improve std::lock algorithm

[Christophe Lyon <christophe.lyon@linaro.org>]

Hi Jonathan,

On Tue, 22 Jun 2021 at 22:34, Jonathan Wakely via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Tue, 22 Jun 2021 at 20:51, Jonathan Wakely wrote:
> >
> > On Tue, 22 Jun 2021 at 17:03, Matthias Kretz <m.kretz@gsi.de> wrote:
> > >
> > > 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?
> >
> > Maybe for the compiler, but I don't think libstdc++ has such a rule. I
> > can add the braces though, it's probably better.
> >
> > >
> > > >               if (!__locks[__i].try_lock())
> > > >                 {
> > > >                   const int __failed = __i;
> > > >                   while (__i--)
> > > >                     __locks[__i].unlock();
> > > >                   return __i;
> > >
> > > You meant `return __failed`?
> >
> > Yep, copy&paste error while trying to avoid the TABs in the real code
> > screwing up the gmail formatting :-(
> >
> >
> > > >                 }
> > > >             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>).
> >
> > Oops, yeah of course.
> >
> > >
> > > > 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.
>
> Here's what I've tested and pushed to trunk. Thanks for the
> improvement and comments.

This patch causes GCC build failures for bare-metal targets
(aarch64-elf, arm-eabi). For instance on arm-eabi, I'm seeing:

In file included from
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/future:38,
                 from
/tmp/1229695_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/precompiled/stdc++.h:105:
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/mutex:
In function 'int std::__detail::__try_lock_impl(_Lockable&)':
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/mutex:522:53:
error: expected primary-expression before ',' token
  522 |         if (unique_lock<_Lockable> __lock{__lockable, try_to_lock})
      |                                                     ^
In file included from
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/future:38,
                 from
/tmp/1229695_7.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/precompiled/stdc++.h:105:
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/mutex:
In function 'int std::__detail::__try_lock_impl(_Lockable&)':
/tmp/1229695_7.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/mutex:522:53:
error: expected primary-expression before ',' token
  522 |         if (unique_lock<_Lockable> __lock{__lockable, try_to_lock})
      |                                                     ^
make[4]: *** [Makefile:1862:
arm-none-eabi/bits/stdc++.h.gch/O2ggnu++0x.gch] Error 1

Can you have a look?

Thanks

Christophe