Introduce a dependency on Abseil #1183

mtrofin · 2021-06-22T16:06:31Z

The main motivation is enabling the benchmark library to play well with
hosts that use Abseil, especially for command line flag functionality.
This also opens up the possibility of reusing functionality such as
string utils, concurrency, or logging.

This was previously discussed in issue #910. This patch:

adds the dependency to Abseil. The library will either be downloaded
(at a fixed, specific commit) or the user may specify their own location
via the ABSEIL_PATH cmake flag
replaces flag functionality with Abseil's.

dmah42 · 2021-06-23T13:56:55Z

I think you need to add a bazel dependency on @com_google_absl in the BUILD file.

cmake/Abseil.cmake

dmah42 · 2021-06-23T15:15:40Z

cmake/Abseil.cmake

+set(ABSL_ENABLE_INSTALL ON)
+if (NOT (DEFINED ABSEIL_PATH))
+  message("ABSEIL_PATH not provided. Downloading.")
+  FetchContent_Declare(


added in 3.11 and we don't require that (to support older linux distributions).

i think you want to follow what the GoogleTest.cmake script does.

#603 for the last discussion about bumping cmake versions.

Ugh. Lots of cmake wizardry. Got it.

dmah42 · 2021-06-23T15:19:45Z

src/commandlineflags.h

-#define DEFINE_kvpairs(name, default_val)         \
-  std::map<std::string, std::string> FLAG(name) = \
-      benchmark::KvPairsFromEnv(#name, default_val)
-
 namespace benchmark {


do we need the rest of this file?

we used to set the default values from the environment... does abseil do that?

I can look into it, my plan was to tackle any more involved logic as a subsequent step; e.g. flag validation (as well as kvpairs)

src/perf_counters.cc

mtrofin · 2021-06-24T15:35:23Z

Punting this to when we move past Ubuntu 14.04 LTS - absl requires a compiler version higher than the version of either gnu/clang on that Linux distro.

oontvoo · 2021-07-29T15:02:16Z

Hi, any idea on exactly when it would be that we move past Ubuntu 14.04 LTS?
It's a pretty old version of ubuntu by now

This patch would benefit a lot of things internally.
Thanks

dmah42 · 2021-09-20T09:24:16Z

https://google.github.io/benchmark/dependencies.html

i believe we can move past 14.04 at this point by quite some way. perhaps we can even drop support for older compilers...

dmah42 · 2021-09-28T08:58:01Z

we now have updated the dependencies, if someone wants to have another go at this. it will up the compiler version, which may mean the removal of some bots that test older versions.

mtrofin · 2021-09-28T16:08:05Z

I can resurrect the patch.

The main motivation is enabling the benchmark library to play well with hosts that use Abseil, especially for command line flag functionality. This also opens up the possibility of reusing functionality such as string utils, concurrency, or logging. This was previously discussed in issue google#910. This patch: - adds the dependency to Abseil. The library will either be downloaded (at a fixed, specific commit) or the user may specify their own location via the ABSEIL_PATH cmake flag - replaces flag functionality with Abseil's.

mtrofin · 2021-09-28T16:53:58Z

reopening

dmah42 · 2021-09-28T16:57:01Z

WORKSPACE

@@ -9,11 +10,10 @@ http_archive(
    sha256 = "d7dc12c1d5bc1a87474de8e3d17b7731a4dcebcfb8aa3990fe8ac7734ef12f2f",
 )

-http_archive(
+git_repository(


https://docs.bazel.build/versions/main/external.html#repository-rules suggests a preference for http_archive

dmah42 · 2021-09-28T16:58:11Z

include/benchmark/benchmark.h

@@ -1338,7 +1341,6 @@ class Fixture : public internal::Benchmark {
 #define BENCHMARK_MAIN()                                                \
  int main(int argc, char** argv) {                                     \
    ::benchmark::Initialize(&argc, argv);                               \


to replicate the previous behaviour we should check that the returned vector is empty.

dmah42 · 2021-09-28T16:59:31Z

src/benchmark.cc

+
+  auto unparsed = absl::ParseCommandLine(*argc, argv);
+  *argc = static_cast<int>(unparsed.size());
+  benchmark::internal::ValidateCommandLineFlags();


nit: don't need the benchmark:: prefix here

oontvoo · 2021-09-28T17:32:07Z

src/mutex.h

-#else
-#define THREAD_ANNOTATION_ATTRIBUTE_(x)  // no-op
-#endif
+#define THREAD_ANNOTATION_ATTRIBUTE_ THREAD_ANNOTATION_ATTRIBUTE__


Not needed?

keeps the changes below simpler. If you prefer, I can remove it and replace THREAD_ANNOTATION_ATTRIBUTE_ with THREAD_ANNOTATION_ATTRIBUTE__

if it's not too much trouble, perhaps it'd be great to do that (since you're already overhauling things here) :)
(Optional request, though)

oontvoo · 2021-09-28T17:34:29Z

src/benchmark.cc

+    bool, benchmark_list_tests, false,
+    "Print a list of benchmarks. This option overrides all other options.");
+
+ABSL_FLAG(std::string, benchmark_filter, ".",


Please change this back to "" (empty).

(We intentionally changed this from "." (ie all) to empty (no benchmarks )to support various internal users)

sorry, must have missed it in the rebase (was going to do a read-over, but wanted to make sure I resurrect the pull first). Thanks for catching this!

LebedevRI · 2021-09-28T17:43:44Z

@mtrofin since you are the one who last updated benchmark in llvm, i feel like this question is uniquely suited for you:
how do you envision the future of the library in llvm?

mtrofin · 2021-09-28T18:18:35Z

@mtrofin since you are the one who last updated benchmark in llvm, i feel like this question is uniquely suited for you: how do you envision the future of the library in llvm?

Not sure that qualifies me in any particular way, so apologies if the following will disappoint :) Also, if it's unrelated to this patch, we can probably start a thread on llvm-discuss or wherever it'd be more appropriate for llvm-test-suite. I remember someone (maybe you) pointing out there are other copies in llvm elsewhere, so maybe we should talk within the community about consolidating them, if there's value in that.

To try and answer your question; for context, I pulled it in llvm-test-suite for 2 reasons: the main one was that I wanted to leverage perf-counters for experiments. The second was the observation that folks were making changes here and then cherry-picking them over to the llvm-test-suite copy, but other features / fixes weren't pulled over to llvm-test-suite (so more code divergence, which seemed like 'a bad thing'). This gives me the limited perspective through which I'd say we'd keep the llvm-test-suite copy sync-ed whenever folks in the second group wanted new features enabled.

This doesn't address the multiple copies of benchmark in llvm, though, nor does it have a reasoned position as to why do it this way (i.e. pull when needed), other than that 'it's similar in spirit to how it's been done so far' (with the benefit of not adding to code divergence). So maybe a more appropriate way would be to sync periodically? WDYT?

LebedevRI · 2021-09-28T18:30:07Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage
this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

oontvoo · 2021-09-28T18:32:58Z

I'm asking because my opinion on this change keeps fluctuating between:

@mtrofin since you are the one who last updated benchmark in llvm, i feel like this question is uniquely suited for you:
how do you envision the future of the library in llvm?

On related topic, with this patch going in, doesn't it mean you'd have to get absl as an LLVM's dependency too? (it isn't nowadays, for various reasons). ... may've opened up a can of worms 🤔

mtrofin · 2021-09-28T18:44:45Z

I'm asking because my opinion on this change keeps fluctuating between:
@mtrofin since you are the one who last updated benchmark in llvm, i feel like this question is uniquely suited for you:
how do you envision the future of the library in llvm?

On related topic, with this patch going in, doesn't it mean you'd have to get absl as an LLVM's dependency too? (it isn't nowadays, for various reasons). ... may've opened up a can of worms 🤔

The dependency would kick in if LLVM_BUILD_BENCHMARKS were enabled; by default, it'd be downloaded as a build dependency.

So I think there would be at least 2 problems:

for folks that want LLVM_BUILD_BENCHMARKS on, but don't want auto-downloads;
if there are build issues or other issues you mentioned
not sure what the precedent is for the former; on the latter, do you know what the issues may be?

oontvoo · 2021-09-29T18:19:37Z

I'm asking because my opinion on this change keeps fluctuating between:
@mtrofin since you are the one who last updated benchmark in llvm, i feel like this question is uniquely suited for you:
how do you envision the future of the library in llvm?

On related topic, with this patch going in, doesn't it mean you'd have to get absl as an LLVM's dependency too? (it isn't nowadays, for various reasons). ... may've opened up a can of worms 🤔

The dependency would kick in if LLVM_BUILD_BENCHMARKS were enabled; by default, it'd be downloaded as a build dependency.

So I think there would be at least 2 problems:

for folks that want LLVM_BUILD_BENCHMARKS on, but don't want auto-downloads;

if there are build issues or other issues you mentioned
not sure what the precedent is for the former; on the latter, do you know what the issues may be?

Nothing concrete ( was thinking of the supported gcc versions ... abseil seems to stop support older gcc sooner but looking at the llvm's doc now, it's basically the same).
So nothing that I can see yet :)

dmah42 · 2021-09-30T09:07:53Z

note: you'll need to remove those failing actions if we can't support 14.04/16.04 any more.

mtrofin · 2021-09-30T15:23:50Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.

@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

dmah42 · 2021-09-30T15:43:55Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.

@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

sgtm

oontvoo · 2021-09-30T16:15:30Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.
@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

sgtm

+1

petrhosek · 2021-10-05T16:56:16Z

cmake/Abseil.cmake

+  message("ABSEIL_PATH not provided. Downloading.")
+  set(ABSEIL_PREFIX "${benchmark_BINARY_DIR}/third_party/abseil")
+  configure_file(${benchmark_SOURCE_DIR}/cmake/Abseil.cmake.in ${ABSEIL_PREFIX}/CMakeLists.txt @ONLY)
+  execute_process(COMMAND ${CMAKE_COMMAND} -G "${CMAKE_GENERATOR}" .


This doesn't pass through CMake options to the Abseil build and instead relies on defaults. For example, CMake will use /usr/bin/cc and /usr/bin/c++ as the default compiler on UNIX-like systems but that may be undesirable. In our case, it's going to fail because those binaries don't exists on our bots at all. Rather, we always specify the compiler manually via CMAKE_C_COMPILER and CMAKE_CXX_COMPILER. The same applies to all other tools. Ideally we would pass all of these variables from the benchmark build to the abseil build.

worth mentioning that other deps in this repo(eg., GoogleTest) also doesn't pass through any build options.

Right. Would that be addressed if we bumped cmake version requirement to 3.11, and thus allow us to use ExternalProject_Add?

petrhosek · 2021-10-05T16:56:56Z

cmake/Abseil.cmake

+if (NOT (DEFINED ABSEIL_PATH))
+  message("ABSEIL_PATH not provided. Downloading.")
+  set(ABSEIL_PREFIX "${benchmark_BINARY_DIR}/third_party/abseil")
+  configure_file(${benchmark_SOURCE_DIR}/cmake/Abseil.cmake.in ${ABSEIL_PREFIX}/CMakeLists.txt @ONLY)


What's the motivation for using the template here rather than including ExternalProject_Add here directly?

see #1183 (review)
(cmake version issues)

gjasny · 2021-10-06T08:58:33Z

Hello,

some time ago I noticed that an LTS abseil used within my project and the webrtc abseil version generate the same symbols despite the goal of abseil to make LTS versions co-linkable with other abseil versions. The filed issue abseil/abseil-cpp#851 does not see much traction.

I could imagine that as long as the issue is not resolved mixing webrtc, grpc, and google-benchmark might lead into an interesting dependency chaos.

Thanks,
Gregor

mtrofin · 2021-10-11T15:49:22Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.
@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

sgtm

+1

LLVM folks were concerned with an abseil dependency, and folks here (e.g. @gjasny being the last one) expressed concerns, too.

@oontvoo, what was the main motivation you'd benefit from the abseil dep? @LebedevRI , you also mentioned a potential upside; @dominichamon , IIRC the main problem we hit when pulling in our repo was around flags, is that correct?

Would this alternative work (@dominichamon , I think you suggested it earlier): we don't require abseil as a dependency; but we factor the problematic APIs, and add build option, such that, if the user has abseil, we use that, else we use our copy. We can start with the problem APIs (flags, and flag macros), and can potentially extend to stuff like string utils and concurrency, but in the same way.

I'd specifically imagine either a subfolder under a third_party one, where we either fork the files we need, or something similar (copy / heavily edit, if compat is hurt)

So this wouldn't require changing the compat matrix. It would require a compat statement as to which version of abseil is known to work (in the case where the user has one and wants to use it instead), and we'd need to add a bot for that.

WDYT?

LebedevRI · 2021-10-11T15:55:57Z

@LebedevRI , you also mentioned a potential upside

My upside was that i would not have to upstream my changes the library once i fork it over this dependency.
Somehow i suspect i'm not the only one, uhm, unhappy about the proposed dependency,
so llvm and whoever else are welcomed to switch upstreams.

For the life of me, i just don't understand why google thinks that forcing changes down people's throats
is a good strategy, but then this is by far not the first time it happens (see numerous google chrome fiaskos).

mtrofin · 2021-10-11T16:00:16Z

@LebedevRI , you also mentioned a potential upside

My upside was that i would not have to upstream my changes the library once i fork it over this dependency. Somehow i suspect i'm not the only one, uhm, unhappy about the proposed dependency, so llvm and whoever else are welcomed to switch upstreams.

For the life of me, i just don't understand why google thinks that forcing changes down people's throats is a good strategy, but then this is by far not the first time it happens (see numerous google chrome fiaskos).

I think we're moving past proposing this dependency, and exploring alternatives that address what motivated it in the first place, without having it as a requirement. IIUC, you'd be fine with such an alternative?

oontvoo · 2021-10-11T16:03:14Z

@oontvoo, what was the main motivation you'd benefit from the abseil dep? [...] @dominichamon , IIRC the main problem we hit when pulling in our repo was around flags, is that correct?

The #1 motivation was flags, yes. (Internally we have an ABSL_FLAG --benchmark_filter, whereas the OSS benchmark here uses its own facility)

Additionally, there are other features from absl that we could benefit, (for eg., most recently: #1230 )

oontvoo · 2021-10-11T16:11:22Z

P.S: I should add that the flag motivation wasn't just a "nice to have" feature - it's almost (*) a requirement for us to switch entirely to the OSS benchmark internally.

(*) of course, there's some hack to get around it - but it'd be great if we could avoid it. As you know, we've already got 2 types of flags (the old command line flags and the newer ABSL_FLAGs)

dmah42 · 2021-10-11T16:18:22Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.
@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

sgtm

+1

LLVM folks were concerned with an abseil dependency, and folks here (e.g. @gjasny being the last one) expressed concerns, too.

@oontvoo, what was the main motivation you'd benefit from the abseil dep? @LebedevRI , you also mentioned a potential upside; @dominichamon , IIRC the main problem we hit when pulling in our repo was around flags, is that correct?

yes though i've worked around that for the most part, but as @oontvoo has suggested already, it's not a great workaround, requiring flags to be plumbed through to benchmark's own flag system instead of using the de facto default ABSL flags.

there's a longer term view of simplifying benchmark to focus on benchmarking...

Would this alternative work (@dominichamon , I think you suggested it earlier): we don't require abseil as a dependency; but we factor the problematic APIs, and add build option, such that, if the user has abseil, we use that, else we use our copy. We can start with the problem APIs (flags, and flag macros), and can potentially extend to stuff like string utils and concurrency, but in the same way.

i think that would be great. it would mean we'd need to keep "our" version in sync with any upstream API changes from third party but that's manageable, i think. i mean, we already manage with things like this...

my concern is it's adding complexity when the long term view should be to reduce complexity. if there's no actual need for abseil, let's just drop it and keep things simple.

I'd specifically imagine either a subfolder under a third_party one, where we either fork the files we need, or something similar (copy / heavily edit, if compat is hurt)

So this wouldn't require changing the compat matrix. It would require a compat statement as to which version of abseil is known to work (in the case where the user has one and wants to use it instead), and we'd need to add a bot for that.

WDYT?

mtrofin · 2021-10-11T16:26:09Z

I'm asking because my opinion on this change keeps fluctuating between:

meh, i'll manage

this greatly simplifies upstreaming for changes! (i.e. either soft fork or hard fork)

I see - IIUC, you're not pushing back with the abseil adoption, but (combining with what @oontvoo was saying) we should consider what we do with the other copy in llvm.
@dominichamon, I think we should circle up to llvm-dev the abseil dep, because of the potential impact to the llvm-proper package. I'll start a thread.

sgtm

+1

LLVM folks were concerned with an abseil dependency, and folks here (e.g. @gjasny being the last one) expressed concerns, too.
@oontvoo, what was the main motivation you'd benefit from the abseil dep? @LebedevRI , you also mentioned a potential upside; @dominichamon , IIRC the main problem we hit when pulling in our repo was around flags, is that correct?

yes though i've worked around that for the most part, but as @oontvoo has suggested already, it's not a great workaround, requiring flags to be plumbed through to benchmark's own flag system instead of using the de facto default ABSL flags.

there's a longer term view of simplifying benchmark to focus on benchmarking...

Would this alternative work (@dominichamon , I think you suggested it earlier): we don't require abseil as a dependency; but we factor the problematic APIs, and add build option, such that, if the user has abseil, we use that, else we use our copy. We can start with the problem APIs (flags, and flag macros), and can potentially extend to stuff like string utils and concurrency, but in the same way.

i think that would be great. it would mean we'd need to keep "our" version in sync with any upstream API changes from third party but that's manageable, i think. i mean, we already manage with things like this...

my concern is it's adding complexity when the long term view should be to reduce complexity. if there's no actual need for abseil, let's just drop it and keep things simple.

SGTM, looks like this PR can be closed, and start a different one along the lines above.

I'd specifically imagine either a subfolder under a third_party one, where we either fork the files we need, or something similar (copy / heavily edit, if compat is hurt)
So this wouldn't require changing the compat matrix. It would require a compat statement as to which version of abseil is known to work (in the case where the user has one and wants to use it instead), and we'd need to add a bot for that.
WDYT?

google-cla bot added the cla: yes label Jun 22, 2021

dmah42 reviewed Jun 23, 2021

View reviewed changes

cmake/Abseil.cmake Outdated Show resolved Hide resolved

dmah42 reviewed Jun 23, 2021

View reviewed changes

pleroy reviewed Jun 23, 2021

View reviewed changes

src/perf_counters.cc Outdated Show resolved Hide resolved

mtrofin closed this Jun 24, 2021

mtrofin added 13 commits September 28, 2021 09:21

Fixed forgotten flag use (only in CHECK statement)

54456e3

Add bazel support

dd4e274

bump cmake version

980199a

Using ExternalProject_Add for abseil, to support cmake pre-3.11

e9bce00

Update argc in Initialize

71316d4

fix &&

0151441

Make benchmark::Initialize return the vector of unparsed flags.

ded4840

better error messaging

2d95753

Add a configure step, hopefully fixes unknown clang flags issue

0524d62

Undo flag config for abseil (upsets windows); detect if flag supported

5b06692

Fix for cmake 3.5.1

95b30ee

Rebased

99cfbdd

mtrofin reopened this Sep 28, 2021

mtrofin force-pushed the main branch from 2f4f989 to 99cfbdd Compare September 28, 2021 16:54

dmah42 reviewed Sep 28, 2021

View reviewed changes

oontvoo suggested changes Sep 28, 2021

View reviewed changes

petrhosek reviewed Oct 5, 2021

View reviewed changes

mtrofin closed this Oct 11, 2021

Introduce a dependency on Abseil #1183

Introduce a dependency on Abseil #1183

Conversation

mtrofin commented Jun 22, 2021

dmah42 commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrofin commented Jun 24, 2021

oontvoo commented Jul 29, 2021

dmah42 commented Sep 20, 2021

dmah42 commented Sep 28, 2021

mtrofin commented Sep 28, 2021

mtrofin commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI commented Sep 28, 2021

mtrofin commented Sep 28, 2021

LebedevRI commented Sep 28, 2021

oontvoo commented Sep 28, 2021

mtrofin commented Sep 28, 2021

oontvoo commented Sep 29, 2021 • edited Loading

dmah42 commented Sep 30, 2021

mtrofin commented Sep 30, 2021

dmah42 commented Sep 30, 2021

oontvoo commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjasny commented Oct 6, 2021

mtrofin commented Oct 11, 2021

LebedevRI commented Oct 11, 2021

mtrofin commented Oct 11, 2021 • edited Loading

oontvoo commented Oct 11, 2021

oontvoo commented Oct 11, 2021

dmah42 commented Oct 11, 2021

mtrofin commented Oct 11, 2021

oontvoo commented Sep 29, 2021 •

edited

Loading

mtrofin commented Oct 11, 2021 •

edited

Loading