From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 516DB384640E for ; Fri, 11 Sep 2020 09:47:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 516DB384640E Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-216-ebduUKjwMX2uZ0nHk3lWSg-1; Fri, 11 Sep 2020 05:47:32 -0400 X-MC-Unique: ebduUKjwMX2uZ0nHk3lWSg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E18758030A3; Fri, 11 Sep 2020 09:47:31 +0000 (UTC) Received: from localhost (unknown [10.33.37.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7D82071775; Fri, 11 Sep 2020 09:47:31 +0000 (UTC) Date: Fri, 11 Sep 2020 10:47:30 +0100 From: Jonathan Wakely To: Mike Crowe Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH v5 5/8] libstdc++ futex: Loop when waiting against arbitrary clock Message-ID: <20200911094730.GL6061@redhat.com> References: <3e4c0ec6864a254cbc58c32abe80983162ea7f16.1590732962.git-series.mac@mcrowe.com> MIME-Version: 1.0 In-Reply-To: <3e4c0ec6864a254cbc58c32abe80983162ea7f16.1590732962.git-series.mac@mcrowe.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Sep 2020 09:47:43 -0000 I'm finally getting round to merging this series! On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >If std::future::wait_until is passed a time point measured against a clock >that is neither std::chrono::steady_clock nor std::chrono::system_clock >then the generic implementation of >__atomic_futex_unsigned::_M_load_when_equal_until is called which >calculates the timeout based on __clock_t and calls the >_M_load_when_equal_until method for that clock to perform the actual wait. > >There's no guarantee that __clock_t is running at the same speed as the >caller's clock, so if the underlying wait times out timeout we need to >check the timeout against the caller's clock again before potentially >looping. > >Also add two extra tests to the testsuite's async.cc: > >* run test03 with steady_clock_copy, which behaves identically to > std::chrono::steady_clock, but isn't std::chrono::steady_clock. This > causes the overload of __atomic_futex_unsigned::_M_load_when_equal_until > that takes an arbitrary clock to be called. > >* invent test04 which uses a deliberately slow running clock in order to > exercise the looping behaviour o > __atomic_futex_unsigned::_M_load_when_equal_until described above. > > * libstdc++-v3/include/bits/atomic_futex.h: > (__atomic_futex_unsigned) Add loop to _M_load_when_equal_until on > generic _Clock to check the timeout against _Clock again after > _M_load_when_equal_until returns indicating a timeout. > > * libstdc++-v3/testsuite/30_threads/async/async.cc: Invent > slow_clock that runs at an eleventh of steady_clock's speed. Use it > to test the user-supplied-clock variant of > __atomic_futex_unsigned::_M_load_when_equal_until works generally > with test03 and loops correctly when the timeout time hasn't been > reached in test04. >--- > libstdc++-v3/include/bits/atomic_futex.h | 15 ++-- > libstdc++-v3/testsuite/30_threads/async/async.cc | 70 +++++++++++++++++- > 2 files changed, 80 insertions(+), 5 deletions(-) > >diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >index 4375129..5f95ade 100644 >--- a/libstdc++-v3/include/bits/atomic_futex.h >+++ b/libstdc++-v3/include/bits/atomic_futex.h >@@ -229,11 +229,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _M_load_when_equal_until(unsigned __val, memory_order __mo, > const chrono::time_point<_Clock, _Duration>& __atime) > { >- const typename _Clock::time_point __c_entry = _Clock::now(); >- const __clock_t::time_point __s_entry = __clock_t::now(); >- const auto __delta = __atime - __c_entry; >- const auto __s_atime = __s_entry + __delta; >- return _M_load_when_equal_until(__val, __mo, __s_atime); >+ typename _Clock::time_point __c_entry = _Clock::now(); >+ do { >+ const __clock_t::time_point __s_entry = __clock_t::now(); >+ const auto __delta = __atime - __c_entry; >+ const auto __s_atime = __s_entry + __delta; >+ if (_M_load_when_equal_until(__val, __mo, __s_atime)) >+ return true; I wonder if we can avoid looping if _Clock::is_steady is true i.e. if _GLIBCXX_CONSTEXPR (_Clock::is_steady) return false If _Clock is steady then it can't be adjusted to move backwards. I am not 100% sure, but I don't think a steady clock can run "slow" either. But I'm checking with LWG about that. We should keep the loop for now. The reason for wanting to return early is that _Clock::now() can be quite expensive, so avoiding the second call if it's not needed would be significant. Related to this, I've been experimenting with a change to wait_for that checks __rtime <= 0 and if so, uses __clock_t::time_point::min() for the wait_until call, so that we don't bother to call __clock_t::now(). For a non-positive duration we don't need to determine the precise time_point in the past, because it's in the past anyway. Using time_point::min() is as good as __clock_t::now() - rtime (as long as we don't overflow anything when converting it to a struct timespec, or course!) Using future.wait_for(0s) to poll for a future becoming ready takes a long time currently: https://wandbox.org/permlink/Z1arsDE7eHW9JrqJ Using wait_until(C::time_point::min()) is faster, because it doesn't call C::now(). This probably isn't important for most timed waiting functions in the library, because I don't see any reason to use wait_for(0s) to poll a mutex, condition_variable or atomic. But it's reasonable to do for futures. I already added (__rtime <= __rtime.zero()) to this_thread::sleep_for in d1a74a287ee1a84b90e5675904dac7f945cffca1. The extra branch to check rtime <= rtime.zero() or atime < C::now() is probably insignificant compared to the cost of unnecessary calls to now() on one or more clocks. >+ __c_entry = _Clock::now(); >+ } while (__c_entry < __atime); >+ return false; > } >