This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add support for benchmark tests using Google Benchmark.
ClosedPublic

Authored by EricWF on Jul 11 2016, 2:34 PM.

Details

Summary

This patch does the following:

  1. Checks in a copy of the Google Benchmark library into the libc++ repo under utils/google-benchmark.
  2. Teaches libc++ how to build Google Benchmark against both (A) in-tree libc++ and (B) the platforms native STL.
  3. Allows performance benchmarks to be built as part of the libc++ build.

Building the benchmarks (and Google Benchmark) is off by default. It must be enabled using the CMake option -DLIBCXX_INCLUDE_BENCHMARKS=ON. When this option is enabled the tests under libcxx/benchmarks can be built using the libcxx-benchmarks target.

On Linux platforms where libstdc++ is the default STL the CMake option -DLIBCXX_BUILD_BENCHMARKS_NATIVE_STDLIB=ON can be used to build each benchmark test against libstdc++ as well. This is useful for comparing performance between standard libraries.

Support for benchmarks is currently very minimal. They must be manually run by the user and there is no mechanism for detecting performance regressions.

Known Issues:

  • -DLIBCXX_INCLUDE_BENCHMARKS=ON is only supported for Clang, and not GCC, since the -stdlib=libc++ option is needed to build Google Benchmark.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 63573.Jul 11 2016, 2:34 PM
EricWF retitled this revision from to [libcxx] Add support for benchmark tests using Google Benchmark..
EricWF updated this object.
EricWF added reviewers: mclow.lists, danalbert, jroelofs.
EricWF added a subscriber: hfinkel.
mclow.lists edited edge metadata.Jul 11 2016, 6:27 PM

My concerns here are not technical - the code and integration here look fine. (and docs!)

But, I wonder about:

  • Licensing. The license is different from everything else in the LLVM tree.
  • Tracking changes to Google Benchmarks. How do we ensure that the included copy doesn't get stale?
  • The CLA stuff in the Google Benchmark code.
EricWF added subscribers: dberlin, chandlerc.

My concerns here are not technical - the code and integration here look fine. (and docs!)

But, I wonder about:

  • Licensing. The license is different from everything else in the LLVM tree.

I discussed this with @chandlerc and this should be OK. LLVM already includes gtest under llvm/utils/unittest/gtest, which has a different license than LLVM.
The original license file for Google Benchmark is kept in the google-benchmark directory, and I noted that the license is different in the README.LLVM.

  • Tracking changes to Google Benchmarks. How do we ensure that the included copy doesn't get stale?

Currently we need to manually update it. The update process is pretty simply. Just cp -R new/ old/.
I was thinking that if LLVM makes the switch to Git we could track it using submodules, but that is quite far off.

  • The CLA stuff in the Google Benchmark code.

I think this should be OK since LLVM also does it with GTest, but I'll confirm.
Hopefully since we shouldn't be taking contributions to google-benchmark internally this won't matter.

Adding @dberlin since he should be able to give a definite answer on these licensing questions.

EricWF updated this revision to Diff 64265.Jul 17 2016, 3:14 PM
EricWF edited edge metadata.

Update unordered_set benchmarks.

EricWF updated this revision to Diff 64279.Jul 18 2016, 12:00 AM

Add benchmarks for std::sort

jroelofs added inline comments.Jul 19 2016, 5:55 AM
benchmarks/CMakeLists.txt
62

Should the standard flag here be "whatever libc++ was compiled with", or does it need to be >= c++14 in order to make the benchmarking library work?

benchmarks/algorithms.bench.cpp
17

Indentation here is different than everywhere else in the file.

docs/BuildingLibcxx.rst
259

Did you add docs for how to run them? Even just saying "run these files in this spot in the build directory" would be helpful between now and if/when LIT gets taught how to run them.

EricWF updated this revision to Diff 64543.Jul 19 2016, 12:27 PM
EricWF marked 2 inline comments as done.

Address @jroelofs comments. Thanks for taking a look.

benchmarks/CMakeLists.txt
62

The benchmark library headers compile in C++03, but I just wanted to keep things current. If we *need* to benchmark C++03/C++11 specific behavior we can change this but until then it's nice to use C++14 features :-)

The dialect we compile libc++ as shouldn't be tied to this. The dylib build is always C++11 because it doesn't need anything more.

docs/BuildingLibcxx.rst
259

I did now :-)

jroelofs added inline comments.Jul 19 2016, 1:44 PM
benchmarks/ContainerBenchmarks.hpp
10

is this the same as benchmark::DoNotOptimize(*data)?

docs/BuildingLibcxx.rst
259

Awesome, thanks!

EricWF added inline comments.Jul 19 2016, 3:51 PM
benchmarks/ContainerBenchmarks.hpp
10

Not sure, I meant to remove it. DoNotOptimize(*data) is defined as asm volatile("" : : "g"(data) : "memory"), so it treats data` as an input parameter. This treats it as an in/out parameter. I was trying to figure out if that made a difference. In particular if it convinced the compiler that data may have changed afterwards.

Anyway I digress. I'll remove it for now.

EricWF updated this revision to Diff 64591.Jul 19 2016, 4:08 PM

Remove Launder. If there are no more comments I would like to commit this tonight.

jroelofs accepted this revision.Jul 19 2016, 4:12 PM
jroelofs edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 19 2016, 4:12 PM
EricWF closed this revision.Jul 19 2016, 4:14 PM