From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 58C053853805 for ; Tue, 22 Jun 2021 19:51:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 58C053853805 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-588-PmBkh3-UOwm0yTrNHF9zJw-1; Tue, 22 Jun 2021 15:51:15 -0400 X-MC-Unique: PmBkh3-UOwm0yTrNHF9zJw-1 Received: by mail-wr1-f69.google.com with SMTP id v8-20020a5d43c80000b029011a94e052f2so1985wrr.2 for ; Tue, 22 Jun 2021 12:51:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JWL9Qh8gpb+xDbBJjNvo+ht1NDTQQaneqU2d7yEwtb8=; b=lkexbzm64f/t5XZ5b30SBEf2pqRh02LctzBFKp198ZD3OIjuLoKUMYMBc7NCRQdt6p J7Q73BmdPdlqzvTopWX7mXpcKTfuXwkT6cAETtQom6myxJVxgsVLzOl7SvyhP7qdjlbb UQLIPdWDPDmF7suVp2lur0ig0Cnby/KwJOdUvWRE1BCYi/GFIgGPOCJZAZqi0micxO5n LH+CexRiw5htbE3IFU5M0Uo5DBLNXZApqVFW3F2TDjmzum5CKTzB1Vrmk2GYfx6hZSZO y0NEWLX7htnsf3+DzZTebqVc/qcrFk7FDLhwE3TTQXjde+q3RlmtCra3YVHkxTREzSB7 yJ0Q== X-Gm-Message-State: AOAM530jWxQhQImzg+9dMtGUeyM6qkg5O6zSNlz/dejP8cZ8Vse4cBss OoIH29wEptwNy2Rq/ZXyK4SDU+QPFsrgL4FnPvDUXCuPtRanuTSu+L/Loxfik7HLwDgrMIr70l1 02ptVDoKJAmqhNc/ZVMIokqTdKr1Ties= X-Received: by 2002:a5d:4b8d:: with SMTP id b13mr6837015wrt.147.1624391474783; Tue, 22 Jun 2021 12:51:14 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzKHEBCDnTEKQOTRhgXh8NHTFEMoT4ZuML6tQbR4LYdfvwDCvkgVSt/NaKu9cCWRLaY/bfOd5sXAWAB/J2brMA= X-Received: by 2002:a5d:4b8d:: with SMTP id b13mr6837002wrt.147.1624391474591; Tue, 22 Jun 2021 12:51:14 -0700 (PDT) MIME-Version: 1.0 References: <14287489.dOz4zN4RiP@minbar> <9108135.T7Z3S40VBb@minbar> In-Reply-To: <9108135.T7Z3S40VBb@minbar> From: Jonathan Wakely Date: Tue, 22 Jun 2021 20:51:03 +0100 Message-ID: Subject: Re: [PATCH v2] libstdc++: Improve std::lock algorithm To: Matthias Kretz Cc: "libstdc++" , gcc Patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Jun 2021 19:51:18 -0000 On Tue, 22 Jun 2021 at 17:03, Matthias Kretz 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 > > 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 . 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, _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.