From patchwork Fri Oct 15 17:29:41 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 46287 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6F266385780F for ; Fri, 15 Oct 2021 17:31:13 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6F266385780F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1634319073; bh=LGZjCVSbPa3s0OPFTXYKZ0ObYWP21xkUdxWAaiyBDfk=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=b016KISPatEaUfytPUudzYapIjJ11E81KbeEBZ59L+UpE34HJ6Ww2Zd6iEnzKEi8x iwebZHHnZckxQJzMFoWuojdY4Ysi+358dMa9PXGpUerb1bv05+sWKxWwQuXyMu9/nM 4RunWc2Eu3u96cWy88zJkNCZ+n7XTMj1MngYHEOM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 703313857C71 for ; Fri, 15 Oct 2021 17:29:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 703313857C71 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-251-Ep5c6yn7Mo-gUuK805zqvQ-1; Fri, 15 Oct 2021 13:29:43 -0400 X-MC-Unique: Ep5c6yn7Mo-gUuK805zqvQ-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 9FB17100CC84; Fri, 15 Oct 2021 17:29:42 +0000 (UTC) Received: from localhost (unknown [10.33.36.131]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4B2505F4E2; Fri, 15 Oct 2021 17:29:42 +0000 (UTC) Date: Fri, 15 Oct 2021 18:29:41 +0100 To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Remove try/catch overhead in std::variant::emplace Message-ID: MIME-Version: 1.0 X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-13.8 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_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" The __variant_construct_by_index helper function sets the new index before constructing the new object. This means that if the construction throws then the exception needs to be caught, so the index can be reset to variant_npos, and then the exception rethrown. This means callers are responsible for restoring the variant's invariants and they need the overhead of a catch handler and a rethrow. If we don't set the index until after construction completes then the invariant is never broken, and callers can ignore the exception and let it propagate. The callers all call _M_reset() first, which sets index to variant_npos as required while the variant is valueless. We need to be slightly careful here, because changing the order of operations in __variant_construct_by_index and removing the try-block from variant::emplace changes an implicit ABI contract between those two functions. If the linker were to create an executable containing an instantiation of the old __variant_construct_by_index and an instantiation of the new variant::emplace code then we would have a combination that breaks the invariant and doesn't have the exception handling to restore it. To avoid this problem, we can rename the __variant_construct_by_index function so that the new emplace code calls a new symbol, and is unaffected by the behaviour of the old symbol. libstdc++-v3/ChangeLog: * include/std/variant (__detail::__variant::__get_storage): Remove unused function. (__variant_construct_by_index): Set index after construction is complete. Rename to ... (__detail::__variant::__construct_by_index): ... this. (variant): Use new name for __variant_construct_by_index friend declaration. Remove __get_storage friend declaration. (variant::emplace): Use new name and remove try-blocks. Tested powerpc64le-linux. Committed to trunk. commit e27771e5dcd8cf2cb757db6177a3485acd28b88f Author: Jonathan Wakely Date: Fri Oct 15 10:58:56 2021 libstdc++: Remove try/catch overhead in std::variant::emplace The __variant_construct_by_index helper function sets the new index before constructing the new object. This means that if the construction throws then the exception needs to be caught, so the index can be reset to variant_npos, and then the exception rethrown. This means callers are responsible for restoring the variant's invariants and they need the overhead of a catch handler and a rethrow. If we don't set the index until after construction completes then the invariant is never broken, and callers can ignore the exception and let it propagate. The callers all call _M_reset() first, which sets index to variant_npos as required while the variant is valueless. We need to be slightly careful here, because changing the order of operations in __variant_construct_by_index and removing the try-block from variant::emplace changes an implicit ABI contract between those two functions. If the linker were to create an executable containing an instantiation of the old __variant_construct_by_index and an instantiation of the new variant::emplace code then we would have a combination that breaks the invariant and doesn't have the exception handling to restore it. To avoid this problem, we can rename the __variant_construct_by_index function so that the new emplace code calls a new symbol, and is unaffected by the behaviour of the old symbol. libstdc++-v3/ChangeLog: * include/std/variant (__detail::__variant::__get_storage): Remove unused function. (__variant_construct_by_index): Set index after construction is complete. Rename to ... (__detail::__variant::__construct_by_index): ... this. (variant): Use new name for __variant_construct_by_index friend declaration. Remove __get_storage friend declaration. (variant::emplace): Use new name and remove try-blocks. diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 4a6826b7ba6..f49094130ee 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -1094,19 +1094,21 @@ namespace __variant >; } -} // namespace __variant -} // namespace __detail - template - void __variant_construct_by_index(_Variant& __v, _Args&&... __args) + inline void + __construct_by_index(_Variant& __v, _Args&&... __args) { - __v._M_index = _Np; auto&& __storage = __detail::__variant::__get<_Np>(__v); ::new ((void*)std::addressof(__storage)) remove_reference_t (std::forward<_Args>(__args)...); + // Construction didn't throw, so can set the new index now: + __v._M_index = _Np; } +} // namespace __variant +} // namespace __detail + template constexpr bool holds_alternative(const variant<_Types...>& __v) noexcept @@ -1342,8 +1344,9 @@ namespace __variant template friend decltype(auto) __variant_cast(_Tp&&); template - friend void __variant_construct_by_index(_Variant& __v, - _Args&&... __args); + friend void + __detail::__variant::__construct_by_index(_Variant& __v, + _Args&&... __args); static_assert(sizeof...(_Types) > 0, "variant must have at least one alternative"); @@ -1507,12 +1510,13 @@ namespace __variant static_assert(_Np < sizeof...(_Types), "The index must be in [0, number of alternatives)"); using type = variant_alternative_t<_Np, variant>; + namespace __variant = std::__detail::__variant; // Provide the strong exception-safety guarantee when possible, // to avoid becoming valueless. if constexpr (is_nothrow_constructible_v) { this->_M_reset(); - __variant_construct_by_index<_Np>(*this, + __variant::__construct_by_index<_Np>(*this, std::forward<_Args>(__args)...); } else if constexpr (is_scalar_v) @@ -1521,9 +1525,9 @@ namespace __variant const type __tmp(std::forward<_Args>(__args)...); // But these steps won't throw: this->_M_reset(); - __variant_construct_by_index<_Np>(*this, __tmp); + __variant::__construct_by_index<_Np>(*this, __tmp); } - else if constexpr (__detail::__variant::_Never_valueless_alt() + else if constexpr (__variant::_Never_valueless_alt() && _Traits::_S_move_assign) { // This construction might throw: @@ -1537,17 +1541,8 @@ namespace __variant // This case only provides the basic exception-safety guarantee, // i.e. the variant can become valueless. this->_M_reset(); - __try - { - __variant_construct_by_index<_Np>(*this, - std::forward<_Args>(__args)...); - } - __catch (...) - { - using __index_type = decltype(this->_M_index); - this->_M_index = static_cast<__index_type>(variant_npos); - __throw_exception_again; - } + __variant::__construct_by_index<_Np>(*this, + std::forward<_Args>(__args)...); } return std::get<_Np>(*this); } @@ -1561,6 +1556,7 @@ namespace __variant static_assert(_Np < sizeof...(_Types), "The index must be in [0, number of alternatives)"); using type = variant_alternative_t<_Np, variant>; + namespace __variant = std::__detail::__variant; // Provide the strong exception-safety guarantee when possible, // to avoid becoming valueless. if constexpr (is_nothrow_constructible_v) { this->_M_reset(); - __variant_construct_by_index<_Np>(*this, __il, + __variant::__construct_by_index<_Np>(*this, __il, std::forward<_Args>(__args)...); } - else if constexpr (__detail::__variant::_Never_valueless_alt() + else if constexpr (__variant::_Never_valueless_alt() && _Traits::_S_move_assign) { // This construction might throw: @@ -1585,17 +1581,8 @@ namespace __variant // This case only provides the basic exception-safety guarantee, // i.e. the variant can become valueless. this->_M_reset(); - __try - { - __variant_construct_by_index<_Np>(*this, __il, - std::forward<_Args>(__args)...); - } - __catch (...) - { - using __index_type = decltype(this->_M_index); - this->_M_index = static_cast<__index_type>(variant_npos); - __throw_exception_again; - } + __variant::__construct_by_index<_Np>(*this, __il, + std::forward<_Args>(__args)...); } return std::get<_Np>(*this); }