Adopts Abseil C++ in Bazel

[looro...@gmail.com]

Hi Bazel devs,

I am proposing to use Abseil C++ in Bazel.

Motivation:

• `//src/main/cpp/util` has a few functions that are the same as Abseil.
  
• By adopting Abseil C++, we can be sure that Bazel is using the most performant and secure implementation of these functions.
• Blaze probably uses internal version of Abseil very heavily. Adopting Abseil can help open-sourcing more Blaze's code faster.
• Protobuf and gRPC will be using Abseil C++ soon. Sooner or later Abseil will end up in Bazel's final binary anyway. Using the same implementation can shrink the binary size too.

Plan:

1. Add @com_google_absl as http_archive in WORKSPACE and live at HEAD (bump version once a week or when a new feature is added).
   
2. Migrate some simpler functions such as blaze_util::safe_strto32 to absl::SimpleAtoi.
   
3. Migrate more //src/main/cpp/util:strings and //src/main/cpp/util:numbers to //absl/strings.
   
4. Migrate usage of stringstream to absl::StrCat and family (does not include wstringstream).
   
5. Look for more places where Abseil's functions can be useful.
6. When Abseil open-sources logging library in late Q3/Q4, migrate logging to Abseil's version (https://groups.google.com/d/msg/abseil-io/SSfWPom44lU/1JLKDF1CBgAJ).

I don't recommend checking-in Abseil into Bazel's source tree as //third_party/abseil-cpp as this will make updating dependency harder. Bazel already uses @com_google_googletest and @bazel_toolchains as http_archive.

Idea on how *one* function will be migrated:

1. Update all call-sites to Abseil's implementation. Mark original implementation as deprecated with compiler-specific attributes to emit compile-time warnings.
2. Announce the change and how to update call-sites somewhere (maybe under this thread?).
   
3. Wait for one week.
   
4. Check for any left over call-sites and clean up. Remove original implementation.

I wonder whether I can request permission to trigger CI build for my PRs (because I can only test Windows build locally)? Chromium will grant external contributors try-job access after they have contributed a few CLs, though it looks like Google projects hosted on Github don't have such practice. From my experience, such permission can really speed up code review process significantly (especially when there is timezone difference between reviewer and contributor).

Note that getting try-job access != becoming committer. I am asking for the first one only.

Thanks for your attention.

Best regards,

Rong Jie

[Marcel Hlopko]

+Dmitry Lomov +Klaus Aehlig +László Csomor 

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.
To post to this group, send email to baze...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/add076c4-2897-4461-911c-2324929286a2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[Marcel Hlopko]

+Lukács T. Berki 

[László Csomor]

Hi Rong Jie,

I agree it'd be nice for Bazel to use Abseil. Migration has a non-zero cost for us though, plus there are other questions.

Questions:

- Rong Jie: sounds like you are volunteering to migrate Bazel's C++ code to use Abseil, is that correct? :)

- Rong Jie: can you say more about which functions you see fit to use from Abseil? String manipulation comes to mind, is there anything else, e.g. file I/O?

- EngProd team: can we grant presubmit execution rights for contributors not in the bazelbuild group? If not, can we create an alternative group for such contributors (who would not have git merge rights, for example).

- Dmitry, Lukacs: do we have a preference or policy on checking in dependencies vs. importing them as an external repo in the WORKSPACE? As far as I can tell, the benefit of checked-in dependencies (like protobuf, ijar, etc.) is that we can clone the Bazel Git repository and build Bazel from that, but updating the dependencies is onerous and we tend to neglect to do it. With external repo rules, we could no longer check out a git repo and just build Bazel, we'd also need to "bazel fetch", but updating the deps would be easier, the git clone would be faster, code base simpler.

Cheers,

Laszlo

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[László Csomor]

+John Field (given that Dmitry and Klaus are on vacation.)

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[Rong Jie Loo]

On Fri, Jun 22, 2018 at 3:33 PM László Csomor <laszlo...@google.com> wrote:

+John Field (given that Dmitry and Klaus are on vacation.)

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

On Fri, Jun 22, 2018 at 9:31 AM László Csomor <laszlo...@google.com> wrote:

Hi Rong Jie,

I agree it'd be nice for Bazel to use Abseil. Migration has a non-zero cost for us though, plus there are other questions.

I understand that. Since Blaze is not open-sourced, it is possible that my change can break internal code.

For example, blaze_util::StringPrintf is replaced with absl::StrFormat(), I think it is reasonable to extend the deprecation period a little longer, or rewrite blaze_util::StringPrintf as a thin wrapper of absl::StrFormat() instead of completely removing it if the engineering effort to change all call-site is too great for internal team.

For newer functions like blaze::ToString that was added *after* Bazel was open-sourced (it was added because some users complained that older GCC does not have working std::to_string), I expect no internal code uses it (because Google has newer compilers), so deprecation period can be shorter.

Questions:

- Rong Jie: sounds like you are volunteering to migrate Bazel's C++ code to use Abseil, is that correct? :)

Yes, I am volunteering for the migration.

- Rong Jie: can you say more about which functions you see fit to use from Abseil? String manipulation comes to mind, is there anything else, e.g. file I/O?

Indeed for now, the migration is more for string manipulation (and Bazel's C++ code *does* deal with a lot of string).

• absl::StrCat, absl::StrAppend (efficient string concatenation and to replace blaze::ToString).
  
• absl::StrJoin to form command line string from vector<string>.
• absl::string_view to view substring without creating new string.
• absl::StartsWith, absl::EndsWith

Some none-string features:

• Wrapper for <algorithm> functions such as absl::c_linear_search(vec, item) (as opposed to std::find(vec.begin(), vec.end(), item) != vec.end()).
• absl::FixedArray and absl::InlinedVector (when we already know the container won't hold too much items)
• Replace std::mutex with absl::Mutex (there is only one usage of std::mutex in Bazel, it is in GrpcBlazeServer class).

I will plan the migration based on how likely it is going to break internal code that I cannot see. (For example, std::mutex -> absl::Mutex should be fairly simple since there is only one usage and it is a private field).

When gRPC and protobuf migrates to Abseil, Bazel will probably need to use absl::string_view, absl::Duration etc to interact with their APIs.

From https://github.com/abseil/abseil.github.io/pull/194, Abseil will probably open-source str_format library to replace <cstdio> on 22 June (US time). It provides printf-like APIs that can handle more C++ types with better performance and type-safety. Some Bazel's error message code uses blaze_util::StringPrintf, so I think the new library is a good fit for Bazel.

Abseil does not have file I/O yet, but their CppCon 2017 talk suggests that they have, just not yet open-sourced.

From User Injection of Filesystems, it seems like Google has a C++ implementation filesystem compatible with C++17 <filesystem> with some extra features. If it is ever included in Abseil, it is worth considering it.

Abseil team estimates that they can open source hash map (open-addressing implementation) in early Q3. I plan to migrate all std::unordered_map (bucket chaining implementation, which is slower) to Abseil's version.

Open-source version of Abseil is still quite young. As an outsider, I can only guess what will be in Abseil in the future, but I can speculate enough things just by looking at Tensorflow/gRPC/Protobuf and other Google projects' base library.

[László Csomor]

Thank you for volunteering, and for the details. This makes sense to me.  I'd be happy to review PRs.

Let me CC +Chloe Calvarin too, who also works on the C++ code of Bazel.

Open questions:
- EngProd team: can we grant CI triggering rights to Rong Jie?
- Lukacs, John, Dmitry: any preference for checking in Absl into Bazel's source tree vs. referencing it from the WORKSPACE? I'm leaning towards the latter, see my earlier email why.

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[Chloé Calvarin]

I believe we will have to check in absl into Bazel's source tree as it is will be part of the main binary and therefore required for bootstrapping Bazel. googletest and others are only required for tests and so are external dependencies.

We were eventually planning on migrating Bazel's c++ client to using absl, for many of the reasons you've identified. We were planning on waiting until absl::logging was finished, to use it as the new backing for BAZEL_LOG, as this would allow us to have platform-appropriate logfiles. Until logging was available, we had decided that the additional library in bazel/third_party would be an unnecessary burden. Using absl's string library is definitely tempting though, so I understand if this decision is reversed.

[László Csomor]

I believe we will have to check in absl into Bazel's source tree as it is will be part of the main binary and therefore required for bootstrapping Bazel.

I believe we only have to include absl in the distribution archive, because that's the only source we support bootstrapping from.

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[Lukács T. Berki]

Technically, we don't need to check Abseil in for bootstrapping because the bootstrap binary we use is Java-only and once that is built, we have the full capabilities of Bazel for the build of the full Bazel binary. However, I would prefer it to be vendored anyway so that we don't deviate from how we treat our other libraries (unless Dmitry thinks this is a good proving ground for our WORKSPACE abilities, but even then, I think it's better to do it in a separate step)

I *think* the original decision not to use what eventually became Abseil was to control our dependencies, but given that we linked in gRPC since then, I think that's a moot point now. So I think using Abseil is better than not and as such, volunteers are *very* appreciated.

A word of warning to Laszlo, though: merging these changes won't be trivial since we also need to update our internal C++ code, too. It's not a lot, but it's not zero work, either.

+Lukács T. Berki 

To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.

To post to this group, send email to baze...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/add076c4-2897-4461-911c-2324929286a2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

--

Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[László Csomor]

So I think using Abseil is better than not and as such, volunteers are *very* appreciated.

YES. Thank you :) 

A word of warning to Laszlo, though: merging these changes won't be trivial since we also need to update our internal C++ code, too. It's not a lot, but it's not zero work, either.

Note taken, thanks!

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

+Lukács T. Berki 

To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.

To post to this group, send email to baze...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/add076c4-2897-4461-911c-2324929286a2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[Laurent Le Brun]

Thanks for volunteering!

But note that most of our code is in Java. The C++ part is relatively small. I don't think the point "Adopting Abseil can help open-sourcing more Blaze's code faster" is true (I can't think of any C++ code we plan to open-source in Bazel). I also suspect that migrating to Abseil will create extra work and bring very little benefits.

Other people more familiar with the C++ code might have a different opinion though. If other people think it's a good use of the time, I will certainly not discourage you.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAJ5fxH%2BduChTsQhSLZK8RaUN9BCT61N4TA1LV%2BrmKcFBhrsBjw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Laurent

[Rong Jie Loo]

I don't object checking-in Abseil into Bazel's source tree, we can do new_local_repository(name = "com_google_absl") and use @com_google_absl//* in deps instead of //third_party/abseil-cpp so that users can override the version of Abseil C++ they want to use.

However, checking-in Abseil will mean adding filegroup(name = "srcs") to every BUILD.bazel file in every Abseil package when updating Abseil and that can be quite tedious. (Maybe write a Python script for this).

Until logging was available, we had decided that the additional library in bazel/third_party would be an unnecessary burden.

I was hoping to start the migration earlier because once Bazel migrates to absl::logging, the team will probably also want to start using other Abseil features as well anywhere. It is better to start the migration when Bazel's C++ codebase isn't too large.

Updated plan of migrating a function.

1. Re-implement old function as thin wrapper of Abseil function. Add comment to suggest that new code should use directly Abseil function instead.
2. Announce the change and how to update call-sites somewhere. (*)
3. Update all call-sites to use Abseil's implementation in open-sourced Bazel. (I know reviewing a refactoring PR is very boring).
4. Allow some time for internal team to update Blaze's code.
5. Mark original implementation as deprecated with compiler-specific attributes to emit compile-time warnings. (If Blaze is built with -Werror, I probably can't do this as they will make the build fails immediately).

1. Check for any left over call-sites and clean up. Remove original implementation.

Basically I will try to make sure that my PR won't immediately break code that I can't see (non-atomic refactoring).

(*) I am thinking about making a Google Sheet with deprecated function, Abseil function to replace, suggested date of removal of old implementation. Then internal team can do clean up when they are free (V8 JavaScript team creates a new "Friday cleanup tracking bug" on some Friday to do some code cleanup).

But note that most of our code is in Java. The C++ part is relatively small. I don't think the point "Adopting Abseil can help open-sourcing more Blaze's code faster" is true (I can't think of any C++ code we plan to open-source in Bazel). I also suspect that migrating to Abseil will create extra work and bring very little benefits.

Performance improvement probably won't be dramatic. In the end, JVM is the main culprit of memory hogging and performance issue. However, there is another advantage of adopting Abseil: security and correctness. Much like other Google projects, Bazel forked older version of Abseil/google3 functions such as blaze_util::StringPrintf for its own use. Abseil/google3 might have fix some bugs in the implementation since then, but Bazel's version remained unpatched.

[Ulf Adams]

Google security team tells me that all source going into the Bazel binary must be code reviewed, and the easiest way to programmatically enforce that is to require that the repo is self contained and has no binary plugs. This may not be the case right now, but will have to be in the future.

That said, I don’t see much benefit from using more of Abseil (note that we already use parts of it). I’m not aware of any remaining C++ code for us to open source, is there?

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAMC1H_pd7eheLjHzGXEYde0YMUSND5CADFF5qVgKZMUUWC82ng%40mail.gmail.com.

[Rong Jie Loo]

note that we already use parts of it

//src/main/cpp/util may have some functions that have similar interface as Abseil C++'s version, but Bazel's implementation is way more simplified (probably due to time constraint). //third_party/py/abseil is Abseil Python library, so that is not really related to this discussion (by the way, the version of Abseil Python library used is 0.1.1 while the latest version is 0.2.2, time to update).

require that the repo is self contained

That is not a bad thing if source files of dependencies are unmodified and they are updated regularly (both are currently not the case for Bazel).

I’m not aware of any remaining C++ code for us to open source, is there?

Other team members in this thread also said the same thing, so I guess that is the case.

[Lukács T. Berki]

On Sat, Jun 23, 2018 at 6:41 PM Ulf Adams <ulf...@google.com> wrote:

Google security team tells me that all source going into the Bazel binary must be code reviewed, and the easiest way to programmatically enforce that is to require that the repo is self contained and has no binary plugs. This may not be the case right now, but will have to be in the future.

That said, I don’t see much benefit from using more of Abseil (note that we already use parts of it).

I think it's reasonable that we don't re-implement these things and i think getting a bit closer ties to another open-source Google project is a reasonable thing to do, especially if protobuf and gRPC are going to adopt it, which seems to be the goal.

I’m not aware of any remaining C++ code for us to open source, is there?

We have a bit of code that deals with the idiosyncrasies of our internal code base. Nothing too dramatic.

[Klaus Aehlig]

> - *Dmitry, Lukacs*: do we have a preference or policy on checking in

> dependencies vs. importing them as an external repo in the WORKSPACE? As
> far as I can tell, the benefit of checked-in dependencies (like protobuf,
> ijar, etc.) is that we can clone the Bazel Git repository and build Bazel
> from that, but updating the dependencies is onerous and we tend to neglect
> to do it.

...provided you have a version of bazel already, and internet access. That
is the state already (since //src:bazel depends on the http_archive
@desugar_jdk_libs already); nevertheless, you can always build offline without
having bazel from the distribution archive, see //:additional_distfiles for
how the files are added and https://github.com/bazelbuild/bazel/issues/5202
for discussion). So adding more libs that way would be just "more of the same",
not a fundamentally new approach.

[László Csomor]

Klaus, sounds like you are arguing for using external repos and against checking in the dependencies, but that's what I was also advocating. (I'm assuming you're answering me, since the quote is from my email.) Am I missing something?

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[Klaus Aehlig]

> Klaus, sounds like you are arguing for using external repos and against
> checking in the dependencies, but that's what I was also advocating.

Actually, I wasn't arguing, just mentioning that even now, you can't
build bazel offline from a checkout, even if you have a bazel already;
I did this, as the text I quoted in my earlier mail seemed to suggest
otherwise.

If ou ask for my personal opinion, yes, it is that we should try to get
our third_party directory empty (or, at least, not add more things to
it) to be very explicit about the distinction between bazel code and
code that is coming from other sources---and to avoid the temptation
to just change third_party code in place (which we did have in the past,
and it was not a good place to be in).

[Dmitry Lomov]

Hi folks,

sorry for the late reply here, I was on vacation for some time. 

The general migration plan looks good.

However, looking at this thread, I am still wondering whether we have a strong reason to add Abseil as a Bazel dependency *at this time*.

The cons of adding the dependency is that:

- it is one more dependency that needs to be maintained and updated

- OSS <-> internal conversion process will need support for remapping between Abseil and internal versions of fucntions

Looking at the original motivation:

> Motivation:

> `//src/main/cpp/util` has a few functions that are the same as Abseil.

Well, yes, but how many functions we are talking about? Do they change much? Why it is beneficial to switch to Abseil implementations? 

> By adopting Abseil C++, we can be sure that Bazel is using the most performant and secure implementation of these functions.

Do we have evidence that current implementations are insufficiently performant or insecure for Bazel?

> Blaze probably uses internal version of Abseil very heavily. Adopting Abseil can help open-sourcing more Blaze's code faster. 

In fact the opposite is true: Bazel does not use "internal version of Abseil" currently, and adopting Abseil will require some remapping between internal and external versions, AFAIU. It is not insurmountable, but it is work that needs to be done.

Also, as others mentioned, there is no non-open-sourced C++ code in Bazel at this time. 

>Protobuf and gRPC will be using Abseil C++ soon. Sooner or later Abseil will end up in Bazel's final binary anyway. Using the same implementation can shrink the binary size too.

What are the sizes savings we are talking about? My guess is that should be < 100Kbytes, probably much less. Does it justify additional dependency for Bazel?

Not to say we shouldn't use Abseil in the future, e.g. for logging when it is available, as Rong Jie Lou and Chloe Calvarin mentioned. 

But for now, I do not see very strong motivation pro Abseil to offset the costs of adding an extra dependency - let me know if I missed or misrepresented anything.

Dmitry

On Mon, Jul 2, 2018 at 1:16 PM Klaus Aehlig <aeh...@google.com> wrote:

--

Google Germany GmbH

Erika-Mann-Straße 33, 80636 München, Germany

[Rong Jie Loo]

On Mon, Jul 2, 2018 at 7:45 PM Dmitry Lomov <dsl...@google.com> wrote:

> `//src/main/cpp/util` has a few functions that are the same as Abseil.

Well, yes, but how many functions we are talking about? Do they change much? Why it is beneficial to switch to Abseil implementations?

 

> By adopting Abseil C++, we can be sure that Bazel is using the most performant and secure implementation of these functions.

Do we have evidence that current implementations are insufficiently performant or insecure for Bazel?

 

One example I can think of: absl::StrCat vs other string formatting utilities. See screenshot at https://youtu.be/xu7q8dGvuwk?t=44m35s for benchmark (Note: Bazel currently uses std::to_string and *stringstream heavily to construct paths and command line).

What are the sizes savings we are talking about? My guess is that should be < 100Kbytes, probably much less. Does it justify additional dependency for Bazel?

If all projects migrate to absl::logging (which Abseil team promises does not use [i]ostream) and every string formatting to no longer depends on *stringstream, each final executable should have about 100KB or more saving if they are fully statically linked to libstdc++ (depends on the number of operator<< overloads, the saving will be less if final executable is dynamically linked to libstdc++). [1] (However, I don't think gRPC and protobuf can be iostream-free anytime soon, migrating existing code to absl::logging and absl/strings won't be quick).

Sharing other Abseil primitives with gRPC and Protobuf probably can push this a little further.

 

I am still wondering whether we have a strong reason to add Abseil as a Bazel dependency *at this time*.

 

Not to say we shouldn't use Abseil in the future, e.g. for logging when it is available, as Rong Jie Loo and Chloe Calvarin mentioned. 

But for now, I do not see very strong motivation pro Abseil to offset the costs of adding an extra dependency - let me know if I missed or misrepresented anything.

 

Since there are no more C++ code to be open-sourced and internal version of Blaze is Abseil-free, starting the migration now or later probably won't make a difference. Therefore, I agree that there is no hurry to add a new dependency now. However, once Bazel has decided to use absl::logging (and maybe together with absl::Status [2]), I think it won't hurt to use more Abseil *after* that?

[1]: [i]ostream and family have long been criticized for their binary footprint as they will pull in the the huge <locale> library into the final binary and linker usually can't eliminate dead code.

[2]: absl::Status can be used to propagate error message back to caller and all the way down instead of just call BAZEL_DIE/diag_err[x] in callee to terminate program. See TODO: https://github.com/bazelbuild/bazel/blob/master/src/main/cpp/blaze_util_windows.cc#L68

[Dmitry Lomov]

On Mon, Jul 2, 2018 at 3:08 PM Rong Jie Loo <looro...@gmail.com> wrote:

On Mon, Jul 2, 2018 at 7:45 PM Dmitry Lomov <dsl...@google.com> wrote:

> `//src/main/cpp/util` has a few functions that are the same as Abseil.

Well, yes, but how many functions we are talking about? Do they change much? Why it is beneficial to switch to Abseil implementations?

 

> By adopting Abseil C++, we can be sure that Bazel is using the most performant and secure implementation of these functions.

Do we have evidence that current implementations are insufficiently performant or insecure for Bazel?

 

One example I can think of: absl::StrCat vs other string formatting utilities. See screenshot at https://youtu.be/xu7q8dGvuwk?t=44m35s for benchmark (Note: Bazel currently uses std::to_string and *stringstream heavily to construct paths and command line).

Certainly absl::StrCat is faster than *stringstream, as many other functions in Abseil - my question is whether it actually makes a difference for Bazel?

What are the sizes savings we are talking about? My guess is that should be < 100Kbytes, probably much less. Does it justify additional dependency for Bazel?

If all projects migrate to absl::logging (which Abseil team promises does not use [i]ostream) and every string formatting to no longer depends on *stringstream, each final executable should have about 100KB or more saving if they are fully statically linked to libstdc++ (depends on the number of operator<< overloads, the saving will be less if final executable is dynamically linked to libstdc++). [1] (However, I don't think gRPC and protobuf can be iostream-free anytime soon, migrating existing code to absl::logging and absl/strings won't be quick).

Sharing other Abseil primitives with gRPC and Protobuf probably can push this a little further.

 

I am still wondering whether we have a strong reason to add Abseil as a Bazel dependency *at this time*.

 

Not to say we shouldn't use Abseil in the future, e.g. for logging when it is available, as Rong Jie Loo and Chloe Calvarin mentioned. 

But for now, I do not see very strong motivation pro Abseil to offset the costs of adding an extra dependency - let me know if I missed or misrepresented anything.

 

Since there are no more C++ code to be open-sourced and internal version of Blaze is Abseil-free, starting the migration now or later probably won't make a difference. Therefore, I agree that there is no hurry to add a new dependency now. However, once Bazel has decided to use absl::logging (and maybe together with absl::Status [2]), I think it won't hurt to use more Abseil *after* that?

Once we do have a dependency then by all means - for any *new* code.

I would caution against change for the sake of change though: if we do not see a clear benefit in rewriting something to use Abseil, we should not do it.

 

[1]: [i]ostream and family have long been criticized for their binary footprint as they will pull in the the huge <locale> library into the final binary and linker usually can't eliminate dead code.

[2]: absl::Status can be used to propagate error message back to caller and all the way down instead of just call BAZEL_DIE/diag_err[x] in callee to terminate program. See TODO: https://github.com/bazelbuild/bazel/blob/master/src/main/cpp/blaze_util_windows.cc#L68

[Julio Merino]

On Mon, Jul 2, 2018 at 3:27 PM, Dmitry Lomov <dsl...@google.com> wrote:

On Mon, Jul 2, 2018 at 3:08 PM Rong Jie Loo <looro...@gmail.com> wrote:

Since there are no more C++ code to be open-sourced and internal version of Blaze is Abseil-free, starting the migration now or later probably won't make a difference. Therefore, I agree that there is no hurry to add a new dependency now. However, once Bazel has decided to use absl::logging (and maybe together with absl::Status [2]), I think it won't hurt to use more Abseil *after* that?

Once we do have a dependency then by all means - for any *new* code.

I would caution against change for the sake of change though: if we do not see a clear benefit in rewriting something to use Abseil, we should not do it.

Fewer custom C++ code on our side, especially low-level / platform-specific code, sounds like a pretty big benefit. Our C++ codebase is quite small and in a rather bad state. Considering that the people in the team that know C++ well are few, reducing the amount of code in this language is worthwhile.

--

Julio Merino / go/jmmv / Blaze

[Dmitry Lomov]

Well we need to offset that by the possibility of introducing bugs during that rewrite. As with any refactoring, testing will make it as fearless as it can be. If we want to do that, we should start with better tests (and that is useful with or without Abseil :))

 

--

Julio Merino / go/jmmv / Blaze

[Chloé Calvarin]

Agreed that the rewrite will need to be done carefully, but I'm with Julio that it's worth it. I've spent a lot of time in the last few months trying to get our c++ libraries to do what they say they do - having libraries that we trust, out of the box? Amazing.

To add another point to the list of reasons to do this - Laszlo's working on the Bazel client's performance when extracting the Bazel binary and starting up the server. In https://github.com/bazelbuild/bazel/pull/5487, he's having to add threadpool to our c++ utilities, where we could just rely on abseil's concurrency support. Can we agree to add abseil directly instead of reimplementing threadpools?

--

You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.
To post to this group, send email to baze...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAObu7DFSWtctKa05v5oRs6zaRHpM48bVpBB5rFnTF-b4Sut4kw%40mail.gmail.com.

[Rong Jie Loo]

On Tue, Jul 3, 2018 at 10:02 PM Chloé Calvarin <ccal...@google.com> wrote:

Agreed that the rewrite will need to be done carefully, but I'm with Julio that it's worth it. I've spent a lot of time in the last few months trying to get our c++ libraries to do what they say they do - having libraries that we trust, out of the box? Amazing.

To add another point to the list of reasons to do this - Laszlo's working on the Bazel client's performance when extracting the Bazel binary and starting up the server. In https://github.com/bazelbuild/bazel/pull/5487, he's having to add threadpool to our c++ utilities, where we could just rely on abseil's concurrency support. Can we agree to add abseil directly instead of reimplementing threadpools?

Open-sourced version of Abseil currently have absl::Mutex, absl::CondVar, absl::once_flag and some other synchronization objects [0], but no thread or threadpool yet. Bazel have to implement its own threadpool for now. A lot of Abseil platform-specific APIs such as filesystem will only be open-sourced after absl::Status is open-sourced [1].

[0]: https://github.com/abseil/abseil-cpp/tree/master/absl/synchronization

[1]: https://github.com/abseil/abseil-cpp/issues/47

[László Csomor]

My bad, I was telling Chloe that ABSL does have a ThreadPool [1], because I overlooked that it was in the "internal" namespace.

[1] https://github.com/abseil/abseil-cpp/blob/master/absl/synchronization/internal/thread_pool.h

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado