-
Notifications
You must be signed in to change notification settings - Fork 15k
[libc++] Add benchmark for std::exception_ptr
#164278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[libc++] Add benchmark for std::exception_ptr
#164278
Conversation
|
@llvm/pr-subscribers-libcxx Author: Adrian Vogelsgesang (vogelsgesang) ChangesThis commit adds benchmarks for Full diff: https://github.com/llvm/llvm-project/pull/164278.diff 1 Files Affected:
diff --git a/libcxx/test/benchmarks/exception_ptr.bench.cpp b/libcxx/test/benchmarks/exception_ptr.bench.cpp
index 7791c510b1eb6..a0c8e9b1d5fba 100644
--- a/libcxx/test/benchmarks/exception_ptr.bench.cpp
+++ b/libcxx/test/benchmarks/exception_ptr.bench.cpp
@@ -18,4 +18,37 @@ void bm_make_exception_ptr(benchmark::State& state) {
}
BENCHMARK(bm_make_exception_ptr)->ThreadRange(1, 8);
+static bool exception_ptr_moves_copies_swap(std::exception_ptr p1) {
+ // Taken from https://github.com/llvm/llvm-project/issues/44892
+ std::exception_ptr p2(p1); // Copy constructor
+ std::exception_ptr p3(std::move(p2)); // Move constructor
+ p2 = std::move(p1); // Move assignment
+ p1 = p2; // Copy assignment
+ swap(p1, p2); // Swap
+ // Comparisons against nullptr. The overhead from creating temporary `exception_ptr`
+ // instances should be optimized out.
+ bool is_null = p1 == nullptr && nullptr == p2;
+ bool is_equal = p1 == p2; // Comparison
+ return is_null && is_equal;
+}
+
+void bm_empty_exception_ptr(benchmark::State& state) {
+ for (auto _ : state) {
+ // All of the `exception_ptr_noops` are no-ops because
+ // the exception_ptr is empty. Hence, the compiler should
+ // be able to optimize them very aggressively.
+ benchmark::DoNotOptimize(exception_ptr_moves_copies_swap(std::exception_ptr{nullptr}));
+ }
+}
+BENCHMARK(bm_empty_exception_ptr);
+
+void bm_exception_ptr(benchmark::State& state) {
+ std::exception_ptr excptr = std::make_exception_ptr(42);
+ for (auto _ : state) {
+ benchmark::DoNotOptimize(excptr);
+ benchmark::DoNotOptimize(exception_ptr_moves_copies_swap(excptr));
+ }
+}
+BENCHMARK(bm_exception_ptr);
+
BENCHMARK_MAIN();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
e4db02b to
9c6d038
Compare
This commit adds benchmarks for `std::exception_ptr` to set a baseline in preparation for follow-up optimizations.
9c6d038 to
47823b9
Compare
|
/libcxx-bot benchmark libcxx/test/benchmarks/exception_ptr.bench.cpp Benchmark results: |
|
(sorry, I was just trying to test the Github action) |
| } | ||
| BENCHMARK(bm_null_exception_ptr); | ||
|
|
||
| void bm_optimized_null_exception_ptr(benchmark::State& state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the purpose of these benchmarks? Perhaps a comment above each would be helpful.
Are these benchmarks going to be useful going forward (e.g. preventing regressions), or are they only useful in the context of comparing stuff as you're actively working on the subsequent patch? bm_optimized_null_exception_ptr seems like it might only be useful to establish a baseline, but might not provide value on its own (but I don't fully understand its purpose yet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the purpose of these benchmarks? Perhaps a comment above each would be helpful.
Updated the comments
Are these benchmarks going to be useful going forward (e.g. preventing regressions), or are they only useful in the context of comparing stuff as you're actively working on the subsequent patch?
I think they might also be useful in the future. E.g., my work in #162773 did not cover the MSVC ABI, because LLVM doesn't have a truly native MSVC exception_ptr implementation, yet. As soon as somebody revisits the Windows exception_ptr implementation, those benchmarks will be useful again.
bm_optimized_null_exception_ptrseems like it might only be useful to establish a baseline, but might not provide value on its own (but I don't fully understand its purpose yet)?
Both bm_optimized_null_exception_ptr and bm_null_exception_ptr check the performance for empty exception_ptrs. The main difference is that for the "optimized" variant, the compiler can proof that the argument passed to exception_ptr_moves_copies_swap is a nullptr, while for the bm_null_exception_ptr the DoNotOptimize(excptr) leaks the exception pointer and the compiler can hence no longer proof that the exception_ptr is empty and must add runtime checks.
The benchmark results for bm_optimized_null_exception_ptr and bm_null_exception_ptr differ slightly, as exemplified by the benchmark result from #164281 (adding move ctor & assignment):
Benchmark Baseline Candidate Difference % Difference
------------------------------- ---------- ----------- ------------ --------------
bm_nonnull_exception_ptr 52.22 40.92 -11.31 -21.65
bm_null_exception_ptr 31.41 23.29 -8.12 -25.85
bm_optimized_null_exception_ptr 28.69 20.50 -8.19 -28.55
Even with #162773 (which inlines much more aggressively, but might never ship in its current state), there still is a slight difference between bm_null_exception_ptr and bm_optimized_null_exception_ptr:
Benchmark Baseline Candidate Difference % Difference
------------------------------- ---------- ----------- ------------ --------------
bm_nonnull_exception_ptr 40.92 33.69 -7.23 -17.66
bm_null_exception_ptr 23.29 1.21 -22.07 -94.79
bm_optimized_null_exception_ptr 20.50 0.70 -19.80 -96.61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the additional explanation and for updating the comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of this as-is. It doesn't benchmark individual functions like we usually do, so it's not at all clear what is actually improved/regressing.
Co-authored-by: Nikolas Klauser <[email protected]>
25817e2 to
b81d32b
Compare
I also added separate benchmarks for individual operations now. However, note that it's not possible to perfectly separate the benchmarks. E.g., Also, the combined |
Yes, perfect separation is sometimes impossible. I'm perfectly fine with that. I'd like to separate unrelated parts though.
Yeah, I guessed as much. However, we can have small snippets which exercise the folds we expect. I'd actually drop some of the P.S. Sorry, I don't mean to be rude, but you're using "proof" wrong here. "to proof something" means "etw. imprägnieren", not "etw. beweisen". The latter would be "to prove something". |
After adding the |
| benchmark::DoNotOptimize(excptr_copy); | ||
| // The compiler should be able to constant-fold the comparison |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the excptr_copy escapes through DoNotOptimize, this comment is incorrect. Throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 right... starring at those test cases for too long already today.
Swapping the two DoNotOptimize calls should fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's wait whether @ldionne has any more comments.
|
I think Louis doesn't have much time currently, so it'll probably take some time until he'd comment. Let's land this for now to unblock the follow-up patches and if he's got any concerns we can address them post-commit. |
This commit adds benchmarks for
std::exception_ptrto set a baseline in preparation for follow-up optimizations.