[libcxx-commits] [libcxx] 66bb6b4 - [libc++] Optimize internal function in <system_error>
Nikolas Klauser via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Aug 11 13:08:58 PDT 2023
Author: Edoardo Sanguineti
Date: 2023-08-11T13:08:45-07:00
New Revision: 66bb6b4fdaedf1c4f5bc19a6fc7d2f5c33aa9471
URL: https://github.com/llvm/llvm-project/commit/66bb6b4fdaedf1c4f5bc19a6fc7d2f5c33aa9471
DIFF: https://github.com/llvm/llvm-project/commit/66bb6b4fdaedf1c4f5bc19a6fc7d2f5c33aa9471.diff
LOG: [libc++] Optimize internal function in <system_error>
In the event the internal function __init is called with an empty string the code will take unnecessary extra steps, in addition, the code generated might be overall greater because, to my understanding, when initializing a string with an empty `const char*` "" (like in this case), the compiler might be unable to deduce the string is indeed empty at compile time and more code is generated.
The goal of this patch is to make a new internal function that will accept just an error code skipping the empty string argument. It should skip the unnecessary steps and in the event `if (ec)` is `false`, it will return an empty string using the correct ctor, avoiding any extra code generation issues.
After the conversation about this patch matured in the libcxx channel on the LLVM Discord server, the patch was analyzed quickly with "Compiler Explorer" and other tools and it was discovered that it does indeed reduce the amount of code generated when using the latest stable clang version (16) which in turn produces faster code.
This patch targets LLVM 18 as it will break the ABI by addressing https://github.com/llvm/llvm-project/issues/63985
Benchmark tests run on other machines as well show in the best case, that the new version without the extra string as an argument performs 10 times faster.
On the buildkite CI run it shows the new code takes less CPU time as well.
In conclusion, the new code should also just appear cleaner because there are fewer checks to do when there is no message.
Reviewed By: #libc, philnik
Spies: emaste, nemanjai, philnik, libcxx-commits
Differential Revision: https://reviews.llvm.org/D155820
Added:
libcxx/benchmarks/system_error.bench.cpp
Modified:
libcxx/benchmarks/CMakeLists.txt
libcxx/docs/ReleaseNotes/18.rst
libcxx/include/__system_error/system_error.h
libcxx/lib/abi/CHANGELOG.TXT
libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist
libcxx/src/system_error.cpp
Removed:
################################################################################
diff --git a/libcxx/benchmarks/CMakeLists.txt b/libcxx/benchmarks/CMakeLists.txt
index 1de1cfa99a812c..b5d3fbf05ed802 100644
--- a/libcxx/benchmarks/CMakeLists.txt
+++ b/libcxx/benchmarks/CMakeLists.txt
@@ -192,6 +192,7 @@ set(BENCHMARK_TESTS
std_format_spec_string_unicode.bench.cpp
string.bench.cpp
stringstream.bench.cpp
+ system_error.bench.cpp
to_chars.bench.cpp
unordered_set_operations.bench.cpp
util_smartptr.bench.cpp
diff --git a/libcxx/benchmarks/system_error.bench.cpp b/libcxx/benchmarks/system_error.bench.cpp
new file mode 100644
index 00000000000000..4b0568d503bc5a
--- /dev/null
+++ b/libcxx/benchmarks/system_error.bench.cpp
@@ -0,0 +1,30 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+#include <system_error>
+
+#include "benchmark/benchmark.h"
+
+static void BM_SystemErrorWithMessage(benchmark::State& state) {
+ for (auto _ : state) {
+ std::error_code ec{};
+ benchmark::DoNotOptimize(std::system_error{ec, ""});
+ }
+}
+BENCHMARK(BM_SystemErrorWithMessage);
+
+static void BM_SystemErrorWithoutMessage(benchmark::State& state) {
+ for (auto _ : state) {
+ std::error_code ec{};
+ benchmark::DoNotOptimize(std::system_error{ec});
+ }
+}
+BENCHMARK(BM_SystemErrorWithoutMessage);
+
+BENCHMARK_MAIN();
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index 8f61227cd23fa9..7dd84a39d9cf77 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -74,6 +74,8 @@ LLVM 18
ABI Affecting Changes
---------------------
+- The symbol of a non-visible function part of ``std::system_error`` was removed.
+ This is not a breaking change as the private function ``__init`` was never referenced internally outside of the dylib
Build System Changes
--------------------
diff --git a/libcxx/include/__system_error/system_error.h b/libcxx/include/__system_error/system_error.h
index bc829491a493f7..b41b73e32a4a21 100644
--- a/libcxx/include/__system_error/system_error.h
+++ b/libcxx/include/__system_error/system_error.h
@@ -36,9 +36,6 @@ class _LIBCPP_EXPORTED_FROM_ABI system_error : public runtime_error {
~system_error() _NOEXCEPT override;
_LIBCPP_HIDE_FROM_ABI const error_code& code() const _NOEXCEPT { return __ec_; }
-
-private:
- static string __init(const error_code&, string);
};
_LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_system_error(int __ev, const char* __what_arg);
diff --git a/libcxx/lib/abi/CHANGELOG.TXT b/libcxx/lib/abi/CHANGELOG.TXT
index f8cc6016d3e095..28503156c87182 100644
--- a/libcxx/lib/abi/CHANGELOG.TXT
+++ b/libcxx/lib/abi/CHANGELOG.TXT
@@ -12,6 +12,25 @@ To generate a summary, re-generate the new ABI list using the
New entries should be added directly below the "Version" header.
+------------
+Version 18.0
+------------
+
+* [libc++] Remove symbol for std::system_error from the dylib
+
+ This patch removes a symbol defined in the library for std::system_error.
+ The symbol '__init' was defined as a private static function as part of the
+ system_error class and was never visible. The addition of this symbol to the ABI was most likely accidental.
+ The function '__init' is replaced by another equivalent function which is placed in the
+ anonymous namespace of the std::system_error source code file.
+ There are no internal references to this symbol which seems to support the reasoning that
+ this was never used outside of the dylib.
+ The deletion of the symbol should not be a breaking change.
+
+All platforms
+ -------------
+ Symbol removed: _ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE
+
------------
Version 17.0
------------
diff --git a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 1c416134cf1f06..a9e1cc53ab83f9 100644
--- a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1062,7 +1062,6 @@
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD2Ev', 'type': 'FUNC'}
-{'is_defined': True, 'name': '__ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 655576abd2586b..d215b6d42dbc6a 100644
--- a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -393,7 +393,6 @@
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD0Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD1Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD2Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
-{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 2046b0c8f8549d..08e80cc3cba390 100644
--- a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -393,7 +393,6 @@
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD0Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD1Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112strstreambufD2Ev', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
-{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 982e94cf6ea070..50ef32c3808410 100644
--- a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1062,7 +1062,6 @@
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112strstreambufD2Ev', 'type': 'FUNC'}
-{'is_defined': True, 'name': '__ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
index bb39b5092b7d4e..01004f1ca38916 100644
--- a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -757,7 +757,6 @@
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD2Ev', 'type': 'FUNC'}
-{'is_defined': True, 'name': '_ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
index ef17138d3d59c6..b341dfbc3e9430 100644
--- a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -755,7 +755,6 @@
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD2Ev', 'type': 'FUNC'}
-{'is_defined': True, 'name': '_ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist
index eccfa1b43ddb7d..a5e66770e5e21f 100644
--- a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist
@@ -727,7 +727,6 @@
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112strstreambufD2Ev', 'type': 'FUNC'}
-{'is_defined': True, 'name': '_ZNSt3__112system_error6__initERKNS_10error_codeENS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeEPKc', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__112system_errorC1ENS_10error_codeERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE', 'type': 'FUNC'}
diff --git a/libcxx/src/system_error.cpp b/libcxx/src/system_error.cpp
index 7875f59d88075b..7800a79370ee70 100644
--- a/libcxx/src/system_error.cpp
+++ b/libcxx/src/system_error.cpp
@@ -59,8 +59,8 @@ error_category::equivalent(const error_code& code, int condition) const noexcept
return *this == code.category() && code.value() == condition;
}
-#if !defined(_LIBCPP_HAS_NO_THREADS)
namespace {
+#if !defined(_LIBCPP_HAS_NO_THREADS)
// GLIBC also uses 1024 as the maximum buffer size internally.
constexpr size_t strerror_buff_size = 1024;
@@ -128,8 +128,26 @@ string do_strerror_r(int ev) {
return string(error_message);
}
#endif
+
+#endif // !defined(_LIBCPP_HAS_NO_THREADS)
+
+string make_error_str(const error_code& ec, string what_arg) {
+ if (ec) {
+ if (!what_arg.empty()) {
+ what_arg += ": ";
+ }
+ what_arg += ec.message();
+ }
+ return what_arg;
+}
+
+string make_error_str(const error_code& ec) {
+ if (ec) {
+ return ec.message();
+ }
+ return string();
+}
} // end namespace
-#endif
string
__do_message::message(int ev) const
@@ -232,50 +250,38 @@ error_code::message() const
// system_error
-string
-system_error::__init(const error_code& ec, string what_arg)
-{
- if (ec)
- {
- if (!what_arg.empty())
- what_arg += ": ";
- what_arg += ec.message();
- }
- return what_arg;
-}
-
system_error::system_error(error_code ec, const string& what_arg)
- : runtime_error(__init(ec, what_arg)),
+ : runtime_error(make_error_str(ec, what_arg)),
__ec_(ec)
{
}
system_error::system_error(error_code ec, const char* what_arg)
- : runtime_error(__init(ec, what_arg)),
+ : runtime_error(make_error_str(ec, what_arg)),
__ec_(ec)
{
}
system_error::system_error(error_code ec)
- : runtime_error(__init(ec, "")),
+ : runtime_error(make_error_str(ec)),
__ec_(ec)
{
}
system_error::system_error(int ev, const error_category& ecat, const string& what_arg)
- : runtime_error(__init(error_code(ev, ecat), what_arg)),
+ : runtime_error(make_error_str(error_code(ev, ecat), what_arg)),
__ec_(error_code(ev, ecat))
{
}
system_error::system_error(int ev, const error_category& ecat, const char* what_arg)
- : runtime_error(__init(error_code(ev, ecat), what_arg)),
+ : runtime_error(make_error_str(error_code(ev, ecat), what_arg)),
__ec_(error_code(ev, ecat))
{
}
system_error::system_error(int ev, const error_category& ecat)
- : runtime_error(__init(error_code(ev, ecat), "")),
+ : runtime_error(make_error_str(error_code(ev, ecat))),
__ec_(error_code(ev, ecat))
{
}
More information about the libcxx-commits
mailing list