This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Granularize algorithm benchmarks
ClosedPublic

Authored by philnik on May 1 2022, 6:07 AM.

Details

Reviewers
Mordante
var-const
ldionne
Group Reviewers
Restricted Project
Commits
rGf94a44767918: [libc++] Granularize algorithm benchmarks

Diff Detail

Event Timeline

philnik created this revision.May 1 2022, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2022, 6:07 AM
Herald added subscribers: mgrang, mgorny. · View Herald Transcript
philnik requested review of this revision.May 1 2022, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2022, 6:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.
If this is the only change, I won't object.

libcxx/benchmarks/algorithms/make_heap.bench.cpp
30

Does an anonymous namespace add value here?

philnik marked 4 inline comments as done.May 6 2022, 1:27 AM
philnik added inline comments.
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.

philnik updated this revision to Diff 427553.May 6 2022, 1:27 AM
philnik marked 2 inline comments as done.
  • Address comments
ldionne accepted this revision.May 18 2022, 1:07 PM

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.

This revision is now accepted and ready to land.May 18 2022, 1:07 PM
philnik updated this revision to Diff 430635.May 19 2022, 3:51 AM
philnik marked an inline comment as done.
  • Remove main macro
This revision was landed with ongoing or failed builds.May 19 2022, 7:14 AM
This revision was automatically updated to reflect the committed changes.