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.129.124]) by sourceware.org (Postfix) with ESMTPS id 20465385B502 for ; Fri, 6 Jan 2023 21:21:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 20465385B502 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673040093; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=svXrH66mpuAIzJKscO6lAPrgIm2l9c0+P+nFZtAE19g=; b=I3Ly5ZFLSITzw2dnUn0N+Z92wR8+6+yYfiZhIS1Ygos5PMmexB/Uk1KfDuPFpE1Gj+E2Hg 1AvhlEpRe/pZQS2EJGlNv9kyRhZAbyAH1eJlJPFNnm8hIrzEgT7Npg80UoCHW8HHFIaeG0 +tU1MYOtYR8H1nQPkZqykipEyTXmp9o= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-315-DuD59JuBNk-b0NPAKCdKWQ-1; Fri, 06 Jan 2023 16:21:30 -0500 X-MC-Unique: DuD59JuBNk-b0NPAKCdKWQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4E761299E759; Fri, 6 Jan 2023 21:21:30 +0000 (UTC) Received: from localhost (unknown [10.33.36.192]) by smtp.corp.redhat.com (Postfix) with ESMTP id 147DD51FF; Fri, 6 Jan 2023 21:21:29 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Refactor time_zone::_Impl::rules_counter [PR108235] Date: Fri, 6 Jan 2023 21:21:29 +0000 Message-Id: <20230106212129.397061-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.2 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_NONE,TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Tested x86_64-linux and powerpc-aix. Pushed to trunk. -- >8 -- Abstract the atomic counter used to synchronize access to time_zone infos behind a Lockable class API, and use atomic_signed_lock_free instead of atomic, as that should be the most efficient type. (For futex-supporting targets it makes no difference, but might benefit other targets in future.) The new API allows the calling code to be simpler, without needing to repeat the same error prone preprocessor conditions in multiple places. It also allows using template metaprogramming to decide whether to use the atomic or a mutex, which gives us more flexibility than only using preprocessor conditions. That allows us to choose the mutex implementation for targets such as hppa-hp-hpux11.11 where 32-bit atomics are not lock-free and so would introduce an unwanted dependency on libatomic. libstdc++-v3/ChangeLog: PR libstdc++/108235 * src/c++20/tzdb.cc (time_zone::_Impl::RulesCounter): New class template and partial specialization for synchronizing access to time_zone::_Impl::infos. (time_zone::_M_get_sys_info, reload_tzdb): Adjust uses of rules_counter. --- libstdc++-v3/src/c++20/tzdb.cc | 137 ++++++++++++++++++++------------- 1 file changed, 84 insertions(+), 53 deletions(-) diff --git a/libstdc++-v3/src/c++20/tzdb.cc b/libstdc++-v3/src/c++20/tzdb.cc index 9103b159400..fa4f4c7a30c 100644 --- a/libstdc++-v3/src/c++20/tzdb.cc +++ b/libstdc++-v3/src/c++20/tzdb.cc @@ -29,7 +29,7 @@ #include // ifstream #include // istringstream #include // ranges::upper_bound, ranges::lower_bound, ranges::sort -#include // atomic, atomic +#include // atomic, atomic #include // atomic> #include // mutex #include // filesystem::read_symlink @@ -598,13 +598,86 @@ namespace std::chrono // Needed to access the list of rules for the time zones. weak_ptr node; -#ifndef __GTHREADS - // Don't need synchronization for accessing the infos vector. -#elif __cpp_lib_atomic_wait - atomic rules_counter; -#else - mutex infos_mutex; + // In the simple case, we don't actual keep count. No concurrent access + // to the infos vector is possible, even if all infos are expanded. + template + struct RulesCounter + { + // Called for each rule-based ZoneInfo added to the infos vector. + // Called when the time_zone::_Impl is created, so no concurrent calls. + void increment() { } + // Called when a rule-based ZoneInfo is expanded. + // The caller must have called lock() for exclusive access to infos. + void decrement() { } + + // Use a mutex to synchronize all access to the infos vector. + mutex infos_mutex; + + void lock() { infos_mutex.lock(); } + void unlock() { infos_mutex.unlock(); } + }; + +#if defined __GTHREADS && __cpp_lib_atomic_wait + // Atomic count of unexpanded ZoneInfo objects in the infos vector. + // Concurrent access is allowed when all objects have been expanded. + // Only use an atomic counter if it would not require libatomic, + // because we don't want libstdc++.so to depend on libatomic. + template requires _Tp::is_always_lock_free + struct RulesCounter<_Tp> + { + atomic_signed_lock_free counter{0}; + + void + increment() + { counter.fetch_add(1, memory_order::relaxed); } + + void + decrement() + { + // The current thread holds the lock, so the counter is negative + // and so we need to increment it to decrement it! + // If the count reaches zero then there are no more unexpanded infos, + // so notify all waiting threads that they can access the infos. + // We must do this here, because unlock() is a no-op if counter==0. + if (++counter == 0) + counter.notify_all(); + } + + void + lock() + { + // If counter is zero then concurrent access is allowed, so lock() + // and unlock() are no-ops and multiple threads can "lock" at once. + // If counter is non-zero then the contents of the infos vector might + // need to be changed, so only one thread is allowed to access it. + for (auto c = counter.load(memory_order::relaxed); c != 0;) + { + // Setting counter to negative means this thread has the lock. + if (c > 0 && counter.compare_exchange_strong(c, -c)) + return; + + if (c < 0) + { + // Counter is negative, another thread already has the lock. + counter.wait(c); + c = counter.load(); + } + } + } + + void + unlock() + { + if (auto c = counter.load(memory_order::relaxed); c < 0) + { + counter.store(-c, memory_order::release); + counter.notify_one(); + } + } + }; #endif + + RulesCounter rules_counter; }; namespace @@ -666,46 +739,8 @@ namespace std::chrono const auto node = _M_impl->node.lock(); auto& infos = _M_impl->infos; -#ifndef __GTHREADS -#elif __cpp_lib_atomic_wait // Prevent concurrent access to _M_impl->infos if it might need to change. - struct Lock - { - Lock(atomic& counter) : counter(counter) - { - // If counter is non-zero then the contents of _M_impl->info might - // need to be changed, so only one thread is allowed to access it. - for (auto c = counter.load(memory_order::relaxed); c != 0;) - { - // Setting counter to negative means this thread has the lock. - if (c > 0 && counter.compare_exchange_strong(c, -c)) - return; - - if (c < 0) - { - // Counter is negative, another thread already has the lock. - counter.wait(c); - c = counter.load(); - } - } - } - - ~Lock() - { - if (auto c = counter.load(memory_order::relaxed); c < 0) - { - counter.store(-c, memory_order::release); - counter.notify_one(); - } - } - - atomic& counter; - }; - Lock lock{_M_impl->rules_counter}; -#else - // Keep it simple, just use a mutex for all access. - lock_guard lock(_M_impl->infos_mutex); -#endif + lock_guard lock(_M_impl->rules_counter); // Find the transition info for the time point. auto i = ranges::upper_bound(infos, tp, ranges::less{}, &ZoneInfo::until); @@ -894,10 +929,8 @@ namespace std::chrono std::rotate(infos.begin() + result_index + 1, i, infos.end()); // Then replace the original rules_info object with new_infos.front(): infos[result_index] = ZoneInfo(new_infos.front()); -#if defined __GTHREADS && __cpp_lib_atomic_wait - if (++_M_impl->rules_counter == 0) // No more unexpanded infos. - _M_impl->rules_counter.notify_all(); -#endif + // Decrement count of rule-based infos (might also release lock). + _M_impl->rules_counter.decrement(); } return info; @@ -1339,11 +1372,9 @@ namespace std::chrono ZoneInfo& info = impl.infos.emplace_back(); is >> info; -#if defined __GTHREADS && __cpp_lib_atomic_wait // Keep count of ZoneInfo objects that refer to named Rules. if (!info.rules().empty()) - impl.rules_counter.fetch_add(1, memory_order::relaxed); -#endif + impl.rules_counter.increment(); } } } -- 2.39.0