From 72d6102e41ad80baa66a2b0f9166be3e8066880b Mon Sep 17 00:00:00 2001 From: Phil Willoughby Date: Wed, 8 Nov 2017 16:38:47 -0800 Subject: [PATCH] Actually mark Unexpected as cold Summary: Testing indicates that GCC ignores the cold attribute when the function is available for inlining. Because Unexpected is a template class we can't make the constructors non-inline, but we can make it derive from a class with a cold constructor, which has the effect of making all the Unexpected constructors implicitly cold. Reviewed By: yfeldblum Differential Revision: D6261013 fbshipit-source-id: 482e49253d5b104742018133c53fb60279dd9f9b --- folly/Expected.h | 9 +++---- folly/Makefile.am | 2 ++ folly/lang/ColdClass.cpp | 18 +++++++++++++ folly/lang/ColdClass.h | 38 +++++++++++++++++++++++++++ folly/lang/test/ColdClassTest.cpp | 43 +++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 folly/lang/ColdClass.cpp create mode 100644 folly/lang/ColdClass.h create mode 100644 folly/lang/test/ColdClassTest.cpp diff --git a/folly/Expected.h b/folly/Expected.h index 2705f335..dcdb378e 100644 --- a/folly/Expected.h +++ b/folly/Expected.h @@ -39,6 +39,7 @@ #include #include #include +#include #define FOLLY_EXPECTED_ID(X) FB_CONCATENATE(FB_CONCATENATE(Folly, X), __LINE__) @@ -675,7 +676,7 @@ namespace expected_detail { * Expected objects in the error state. */ template -class Unexpected final { +class Unexpected final : ColdClass { template friend class Unexpected; template @@ -712,10 +713,8 @@ class Unexpected final { Unexpected(Unexpected&&) = default; Unexpected& operator=(const Unexpected&) = default; Unexpected& operator=(Unexpected&&) = default; - FOLLY_COLD constexpr /* implicit */ Unexpected(const Error& err) - : error_(err) {} - FOLLY_COLD constexpr /* implicit */ Unexpected(Error&& err) - : error_(std::move(err)) {} + constexpr /* implicit */ Unexpected(const Error& err) : error_(err) {} + constexpr /* implicit */ Unexpected(Error&& err) : error_(std::move(err)) {} template ::value)> diff --git a/folly/Makefile.am b/folly/Makefile.am index 422b7bb8..83f79c20 100644 --- a/folly/Makefile.am +++ b/folly/Makefile.am @@ -313,6 +313,7 @@ nobase_follyinclude_HEADERS = \ io/async/test/Util.h \ json.h \ lang/Assume.h \ + lang/ColdClass.h \ lang/Launder.h \ lang/RValueReferenceWrapper.h \ lang/SafeAssert.h \ @@ -566,6 +567,7 @@ libfolly_la_SOURCES = \ io/async/ssl/SSLErrors.cpp \ json.cpp \ lang/Assume.cpp \ + lang/ColdClass.cpp \ lang/SafeAssert.cpp \ detail/MemoryIdler.cpp \ detail/SocketFastOpen.cpp \ diff --git a/folly/lang/ColdClass.cpp b/folly/lang/ColdClass.cpp new file mode 100644 index 00000000..cd1827fd --- /dev/null +++ b/folly/lang/ColdClass.cpp @@ -0,0 +1,18 @@ +/* + * Copyright 2004-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "ColdClass.h" + +folly::cold_detail::ColdClass::ColdClass() noexcept {} diff --git a/folly/lang/ColdClass.h b/folly/lang/ColdClass.h new file mode 100644 index 00000000..25d266fb --- /dev/null +++ b/folly/lang/ColdClass.h @@ -0,0 +1,38 @@ +/* + * Copyright 2004-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* Tag any class as `cold` by inheriting from folly::cold::ColdClass + * + * Intended use: things like folly::Unexpected which are designed to only be + * instantiated on error paths. + */ +#pragma once + +#include + +namespace folly { +// ColdClass should be in its own namespace: inheriting from any class adds its +// innermost namespace to the namespaces inspected during +// argument-dependent-lookoup. We want people to be able to derive from this +// without implicitly picking up the folly namespace for ADL on their classes. +namespace cold_detail { +struct ColdClass { + FOLLY_COLD ColdClass() noexcept; +}; +} // namespace cold + +/* using override */ using cold_detail::ColdClass; +} // namespace folly diff --git a/folly/lang/test/ColdClassTest.cpp b/folly/lang/test/ColdClassTest.cpp new file mode 100644 index 00000000..30d70ce9 --- /dev/null +++ b/folly/lang/test/ColdClassTest.cpp @@ -0,0 +1,43 @@ +/* + * Copyright 2004-present Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +using folly::ColdClass; + +TEST(ColdClass, inheritance) { + // The only verifiable property of ColdClass is that it must not disrupt the + // default constructor/destructor, default copy/move constructors and default + // copy/move assignment operators when a class derives from it. + struct TestStruct : ColdClass {}; + EXPECT_TRUE(std::is_nothrow_default_constructible::value); + EXPECT_TRUE(std::is_trivially_copy_constructible::value); + EXPECT_TRUE(std::is_trivially_move_constructible::value); + EXPECT_TRUE(std::is_trivially_copy_assignable::value); + EXPECT_TRUE(std::is_trivially_move_assignable::value); + EXPECT_TRUE(std::is_trivially_destructible::value); + // Same again, but private inheritance. Should make no difference. + class TestClass : ColdClass {}; + EXPECT_TRUE(std::is_nothrow_default_constructible::value); + EXPECT_TRUE(std::is_trivially_copy_constructible::value); + EXPECT_TRUE(std::is_trivially_move_constructible::value); + EXPECT_TRUE(std::is_trivially_copy_assignable::value); + EXPECT_TRUE(std::is_trivially_move_assignable::value); + EXPECT_TRUE(std::is_trivially_destructible::value); +} -- 2.34.1