This is an archive of the discontinued LLVM Phabricator instance.

Updating the `benchmark` dependency of Microbenchmarks
ClosedPublic

Authored by mtrofin on Apr 28 2021, 11:01 AM.

Details

Summary

Updating the dependency to enable leveraging perf counter support.

Renamed the directory to be version-independent. This would allow us to make updates when needed, without worrying about the relation to the directory name version. One benefit is that we can avoid cherry-picking needed changes.

Diff Detail

Repository
rT test-suite

Event Timeline

mtrofin created this revision.Apr 28 2021, 11:01 AM
mtrofin requested review of this revision.Apr 28 2021, 11:01 AM

should we do this with submodules, and then avoid these large commits?

should we do this with submodules, and then avoid these large commits?

This was not endorsed when I added the gcc torture suite, see the discussion here: https://lists.llvm.org/pipermail/llvm-dev/2019-October/135768.html

The only thing you've missed is updating LICENSE.TXT in the root of the repo: there is a line near the bottom to say that benchmark has its own repo/copyright, it seems not to have been updated in a while.

mtrofin updated this revision to Diff 341517.Apr 29 2021, 7:51 AM

updated license

should we do this with submodules, and then avoid these large commits?

This was not endorsed when I added the gcc torture suite, see the discussion here: https://lists.llvm.org/pipermail/llvm-dev/2019-October/135768.html

Can't seem to find that part of the discussion - do you remember what the motivation was by any chance? Maybe it was about build bot scripts? (I'm not feeling too strongly about submodules, just curious)

The only thing you've missed is updating LICENSE.TXT in the root of the repo: there is a line near the bottom to say that benchmark has its own repo/copyright, it seems not to have been updated in a while.

Thanks, done - also pointed to benchmark/googletest, which has its own LICENSE file, too

Please note that there are two more copies of benchmark in monorepo - in llvm and in libcxx
I think updating may be good, but i'm not really sold at living at head for it.

Please note that there are two more copies of benchmark in monorepo - in llvm and in libcxx

Ack - I can update those, too (separate patches). Fortunately, those use a generic dir name, so should be a simpler change.

I think updating may be good, but i'm not really sold at living at head for it.

I also don't think we should live at head; but having the name of the dir version-independent would be conducive to making updates in-place and simpler to read.
I'm more imagining infrequent (1-2 per year) updates, judging by the updates encountered in the past 3-4 years. The trigger would be a patch in benchmark upstream that folks need. I'd propose preferring doing an update then, rather than cherrypick - wdyt?

mtrofin updated this revision to Diff 346507.May 19 2021, 10:49 AM

updated, also added a helper script.

Gentle reminder - thanks!

The patch description says what it does, but not why it does what it does.

mtrofin edited the summary of this revision. (Show Details)May 24 2021, 10:00 AM

The patch description says what it does, but not why it does what it does.

Done.

lenary accepted this revision.May 24 2021, 10:13 AM
This revision is now accepted and ready to land.May 24 2021, 10:13 AM

Not clear why we're going with not updating in place? Or just trying to do this in multiple stages?

Proposing using a version-independent directory name, and performing more frequent updates. This should help avoid doing cherrypicks here (which was the subject of changes since the last update).

I mean, you can propose it by actually proposing it. Doesn't feel like there's a proposal in this commit.

mtrofin updated this revision to Diff 347448.May 24 2021, 10:29 AM

also removed old benchmarks-1.3.0, and updated commit message

mtrofin edited the summary of this revision. (Show Details)May 24 2021, 10:29 AM

Not clear why we're going with not updating in place? Or just trying to do this in multiple stages?

Proposing using a version-independent directory name, and performing more frequent updates. This should help avoid doing cherrypicks here (which was the subject of changes since the last update).

I mean, you can propose it by actually proposing it. Doesn't feel like there's a proposal in this commit.

The old directory name contained a version ('benchmark-1.3.0'); but yes, probably makes sense to just do it. So updated the patch with the removal of the old one, and rephrased a bit the commit message.

This revision was automatically updated to reflect the committed changes.

This patch appears to be causing timing problems of the sort that the calls to test_suite_add_build_dependencies in MicroBenchmarks/libs/CMakeLists.txt were meant to address.

[  0%] Building CXX object MicroBenchmarks/libs/benchmark/third_party/googletest/build/googletest/CMakeFiles/gtest.dir/src/gtest-all.cc.o
/bin/sh: /home/comptst/worker/LLVM-Master-RHEL-Release/testlog/test-suite-build/tools/timeit: No such file or directory
make[2]: *** [MicroBenchmarks/libs/benchmark/third_party/googletest/build/googletest/CMakeFiles/gtest.dir/src/gtest-all.cc.o] Error 127

Looking;

do you happen to remember the context of "the sort that the calls to
test_suite_add_build_dependencies in
MicroBenchmarks/libs/CMakeLists.txt were meant to address." - i.e. a
pointer to a discussion detailing the problem, so I can better understand
it?

Thanks!

do you happen to remember the context of "the sort that the calls to
test_suite_add_build_dependencies in
MicroBenchmarks/libs/CMakeLists.txt were meant to address." - i.e. a
pointer to a discussion detailing the problem, so I can better understand
it?

I wasn't actively involved in that discussion. It is here though: https://reviews.llvm.org/D32272#797060.

Thanks!

np

Fixed - please verify (and sorry for the nuisance)

The breakage was due to new test targets being brought in by benchmark. We
don't actually care about those - we only need the benchmark target. So
it's simpler to just disable benchmark's test targets wholesale.

Fixed - please verify (and sorry for the nuisance)

Thanks (and no worries). Looks like I'll have to wait for some delay in our internal CI before being able to confirm.

The breakage was due to new test targets being brought in by benchmark. We
don't actually care about those - we only need the benchmark target. So
it's simpler to just disable benchmark's test targets wholesale.

Interesting. Thanks for the explanation.

Fixed - please verify (and sorry for the nuisance)

Thanks (and no worries). Looks like I'll have to wait for some delay in our internal CI before being able to confirm.

I have verified that this issue has been resolved. Thanks again.