Details
- Reviewers
Mordante var-const ldionne - Group Reviewers
Restricted Project - Commits
- rGf94a44767918: [libc++] Granularize algorithm benchmarks
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general looks like an improvement, but I have some questions and remarks.
libcxx/benchmarks/CMakeLists.txt | ||
---|---|---|
191 | nice no more globbing! | |
213 | Can you explain why this is done? | |
libcxx/benchmarks/algorithms.bench.cpp | ||
1 | Can you use common.h instead of Common.h? | |
62 | Please try to do these changes in a separate commit and not while moving code. | |
libcxx/benchmarks/algorithms/make_heap.bench.cpp | ||
30 | Does an anonymous namespace add value here? |
libcxx/benchmarks/CMakeLists.txt | ||
---|---|---|
213 | Otherwise it would try to compile make_heap.bench.cpp and not algorithms/make_heap.bench.cpp, which it obviously doesn't find. | |
libcxx/benchmarks/algorithms.bench.cpp | ||
62 | Yes, this was the only code change (and the inline added below). It's just to silence a warning. The warnings was there before, but now it's duplicated for every file in benchmark/. | |
libcxx/benchmarks/algorithms/make_heap.bench.cpp | ||
30 | With an anonymous namespace the compiler is much more likely if the function is only used in a single place to inline stuff because it could remove the function then. The cost of inlining an empty functions is -15030, so probably almost no function is emitted when it's in an anonymous namespace and used only once. I don't know if it makes any performance difference in these cases though. |
LGTM with my comment applied.
libcxx/benchmarks/algorithms/common.h | ||
---|---|---|
244 | I think I'd rather not introduce this macro since the other benchmarks don't use it. IMO the cost of this duplication is not enough to try avoiding it with a macro. |
nice no more globbing!