absl TBD features past their two-year limit

251 views
Skip to first unread message

Peter Kasting

unread,
Dec 17, 2024, 8:52:20 AM12/17/24
to cxx
We have four sections in the Abseil TBD features list past the two-year point, so we should make calls on them. The four are AnyInvocable, containers, CRC32C, and logging.

Starting point proposal:
  1. AnyInvocable: Ban. I don't see any advantage to using this if we have FunctionRef and Callback.
  2. Containers: Allow. I know dcheng@ has wanted to give guidance on this and then allow for a long time, but he's perpetually flooded; let's just allow first and guide later.
  3. CRC32C: Allow, and file a bug to migrate existing third_party/crc32c to using (and drop that lib). Would like to hear from affected parties, though (security? net?).
  4. Logging: Uh... Ban but file a bug to drop logging.h in favor of this and allow? I don't know what the subtle gotchas are here, but I don't know why we have our own impl. Seems like we shouldn't just freely allow intermixing in the meantime. Maybe we can though? Anyone look at this? pbos, dcheng?
PK

Harald Alvestrand

unread,
Dec 17, 2024, 9:03:12 AM12/17/24
to Peter Kasting, cxx
FWIW, WebRTC has decided to allow AnyInvocable, and is using it extensively, in preference to alternative constructs; the fact that you can safely store an AnyInvocable means that we use it where Chrome probably would be using a RepeatingCallback.

WebRTC is not supposed to depend on Chromium's /base, so we don't have FunctionRef or OnceCallback / RepeatingCallback.





--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/c88209fe-3127-4164-a811-2b8e2db9e721n%40chromium.org.

Peter Kasting

unread,
Dec 17, 2024, 1:30:33 PM12/17/24
to Harald Alvestrand, cxx
On Tue, Dec 17, 2024 at 1:03 AM Harald Alvestrand <h...@google.com> wrote:
FWIW, WebRTC has decided to allow AnyInvocable, and is using it extensively, in preference to alternative constructs; the fact that you can safely store an AnyInvocable means that we use it where Chrome probably would be using a RepeatingCallback.

WebRTC is not supposed to depend on Chromium's /base, so we don't have FunctionRef or OnceCallback / RepeatingCallback.

Yeah, it's extremely useful when you don't have //base. google3 uses it extensively and it sounds like it's the right thing for WebRTC also.

You don't consider yourselves bound by our styleguide/features list, do you?

PK 

Nico Weber

unread,
Dec 17, 2024, 3:29:19 PM12/17/24
to Peter Kasting, cxx
Before dropping logging.h, please carefully look at build time impact. logging.h is included very often, and absl headers tend to be somewhat expensive. Also, it seems nice to have control over these files (see e.g. NOTREACHED() effort). So I think I'm in favor of Ban and no follow-up bug here.

Harald Alvestrand

unread,
Dec 17, 2024, 4:13:49 PM12/17/24
to Peter Kasting, cxx
Our styleguide says "we follow Google's and Chrome's styleguide, except when we don't, and here are the places we don't". And then we add some additional guidance on top of that, but that's more like "recommended usages where the others are silent".

Peter Kasting

unread,
Dec 18, 2024, 8:03:36 PM12/18/24
to cxx, h...@google.com, cxx, Peter Kasting
On Tuesday, December 17, 2024 at 8:13:49 AM UTC-8 h...@google.com wrote:
Our styleguide says "we follow Google's and Chrome's styleguide, except when we don't, and here are the places we don't".

Since AnyInvocable is TBD today, it's already banned-in-practice in Chromium. And looking at the WebRTC docs, https://webrtc.googlesource.com/src/+/HEAD/g3doc/abseil-in-webrtc.md explicitly allows it. So I think WebRTC is insulated from any change we make here.

PK

Peter Kasting

unread,
Dec 18, 2024, 8:05:56 PM12/18/24
to cxx, Nico Weber, cxx, Peter Kasting
On Tuesday, December 17, 2024 at 7:29:19 AM UTC-8 Nico Weber wrote:
On Tue, Dec 17, 2024 at 3:52 AM Peter Kasting <pkas...@chromium.org> wrote:
  1. Logging: Uh... Ban but file a bug to drop logging.h in favor of this and allow? I don't know what the subtle gotchas are here, but I don't know why we have our own impl. Seems like we shouldn't just freely allow intermixing in the meantime. Maybe we can though? Anyone look at this? pbos, dcheng?
Before dropping logging.h, please carefully look at build time impact. logging.h is included very often, and absl headers tend to be somewhat expensive. Also, it seems nice to have control over these files (see e.g. NOTREACHED() effort). So I think I'm in favor of Ban and no follow-up bug here.

Yeah, good call.

I wonder if the right compromise is "ban, with a followup bug suggesting investigating the build time impact". The controlling-our-destiny thing matters too, but since logging.h is only used for LOG (which we're supposed to minimally use) and not [D]CHECK or NOTREACHED (which we are aggressively changing), I wonder if it matters enough to keep this -- it feels like if it does, then maybe we shouldn't use any Abseil at all.

PK

Peter Boström

unread,
Dec 18, 2024, 11:26:55 PM12/18/24
to Peter Kasting, cxx, Nico Weber
I think there may be some marginal value in minimizing differences between absl and base/ logging up to where one could (but doesn't need to) be a drop-in replacement for the other. It may also make importing things (incl from google-internal) easier in the future.

I'd also accept having some "basic" version of logging that's absl compatible, and have our own intentional-differences stuff be split out from the main base/logging.h headers. I'm not sure doing this investigation or split-out work is worthwhile but with infinite time and resources sure.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2cf1c28d-c100-4ee3-bb9c-564ea75dad2an%40chromium.org.

David Benjamin

unread,
Dec 18, 2024, 11:48:48 PM12/18/24
to Peter Boström, Peter Kasting, cxx, Nico Weber, Victor Vasiliev
QUICHE has a mess of overrides and abstractions due to differences like this. I think we should *at least* get to the point that we're happy with dependencies using Abseil logging. Whether that's registering a log sink, or some further convergence, or maybe it's already fine, I'm not sure.

Of course this is one of those "I think that would be nice, but I'm not volunteering to do it or suggesting its a priority or anything" kinds of "I think we should"s. :-) But, I dunno, my 2 cents if anyone feels excited about logging.

Peter Kasting

unread,
Dec 30, 2024, 6:30:51 PM12/30/24
to cxx, Peter Kasting
There have been responses re: AnyInvocable (which was mostly about whether our ban would impact WebRTC; I think the answer is no) and Logging (which sound in favor of banning for now, and weak consensus on leaving a P3 bug open about looking into this more deeply, probably with some of the discussion here for context).

No response on Containers and CRC32. Any thoughts on those?

I realize it is the holidays, so how about we try to wrap this up in about two weeks? That will give us the full week after many people return next week.

PK

Daniel Cheng

unread,
Dec 31, 2024, 5:44:47 AM12/31/24
to Peter Kasting, cxx
I'm fine allowing the hash table containers. They exhibit some binary size regressions, but even despite that, I believe absl::flat_hash_map/absl::flat_hash_set are a better default container choice than base::flat_map.

However:
- In the long-term, we should migrate all std::unordered_map/std::unordered_set to at least absl::node_hash_map/absl::node_hash_set. This is pretty straightforward. I don't think it should be blocking, but it should happen at some point because:
- We need to update the guidance in //base/containers (ideally it might represent something like the guidance in go/totw/183). We shouldn't have both the STL unordered containers and the absl unordered containers in the decision tree; we should only have the absl unordered containers. (Since this is Chrome, I suspect we'll probably need unordered_{map,set} in some places for compat with 3p code)
- To simplify the job for whoever updates the containers documentation, drop the exact binary size numbers for now. We can add them back when someone figures out a "representative" snippet of code that can be reproducibly tested for binary size numbers.
- Finally, there should be guidance around defining hashing operations, even if it's just "prefer the absl way <insert example(s)>" and "std::hash if you really need to use std::unordered_map for compatibility with 3p code"

I would love to allow the btree containers, as they are generally better than std::map. However, they have a very high binary size cost: the last time I checked, it was near a 1MB increase to change all std::map/std::set to absl::btree_map/absl::btree_set... in //content, so this is just a subset of the maps in Chrome.

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/bf95b8c3-210a-46e5-b429-5597c19a43d2n%40chromium.org.

Adam Rice

unread,
Jan 5, 2025, 3:54:54 PMJan 5
to Daniel Cheng, Peter Kasting, cxx
My two cents:
  1. AnyInvocable: Ban. (same as pkasting)
  2. Containers: Allow. (same as pkasting)
  3. CRC32C: Benchmark then Allow.
  4. Logging: Ban and add nudges to our macros to discourage the non-D versions.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrcUX7WZMkJVaGo8a52O-DnwT_nxadieGCCLh2M0fh8xg%40mail.gmail.com.

Peter Kasting

unread,
Jan 5, 2025, 6:03:52 PMJan 5
to Adam Rice, Daniel Cheng, cxx
Is there appropriate crc32c benchmarking somewhere we could use? If not I don't have the expertise to write any.

PK

Chris Blume

unread,
Jan 5, 2025, 6:10:04 PMJan 5
to Peter Kasting, Adenilson Cavalcanti, Adam Rice, Daniel Cheng, cxx
Zlib uses crc32c I believe. Adenilson has done a lot of zlib component benchmarking and optimization, including the CRC.


Adenilson, is there a standard benchmark we could run? I think you have a manual one?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBNzLpU_OOJ4LZKk6NdJXumoU3kFckch2M_DK4TbGXQgw%40mail.gmail.com.

Adenilson Cavalcanti

unread,
Jan 5, 2025, 9:13:58 PMJan 5
to Chris Blume, Peter Kasting, Adam Rice, Daniel Cheng, cxx
I think I need a bit more context on *which* crc32c is being discussed.

Is it the one in abseil*, the one in third-party in Chromium** or the
ones we implemented in
Chromium zlib***? Something else?

The one in abseil already seems to have a benchmark of some sort
(https://github.com/abseil/abseil-cpp/blob/master/absl/crc/crc32c_benchmark.cc).

Concerning Chromium zlib, I initially measured the speed using a hash
perf test I wrote when I optimized the hashes used in Chromium like
SHA1 (https://source.chromium.org/chromium/chromium/src/+/main:base/hash/hash_perftest.cc).

Lately I measure it indirectly by calculating the decompression speed
of GZIP content (i.e. MB/s, a faster hash will generally help with
it),
but I think I may have a benchmarker somewhere in my laptop but never
bothered to publish it.

It is also important to differentiate *CRC32* vs *CRC32C*
(Castagnoli). The first is used in GZIP (thus implemented in zlib)
and the second apparently for iSCSI.

The basic difference is the polynomial used. I'm unsure what is really
required by Abseil (maybe someone could clarify?).

I never quite understood why there is such proliferation of multiple
implementations of CRC32 given that we already implemented
it way back in 2018 for both armv8/aarch64 and x86-64 (added support
recently for AVX-512 too) and Chromium zlib sits way below in the
software stack as dependency (e.g. Chromium, Android, V8) so it could
be simply reused.

Apparently the Abseil code was written in 2022, but it may be the case
that it has special requirements (e.g. another polynomial or
maybe code size, dunno).

* https://github.com/abseil/abseil-cpp/tree/master/absl/crc
** https://source.chromium.org/chromium/chromium/src/+/main:third_party/crc32c/
*** https://source.chromium.org/chromium/chromium/src/+/main:third_party/zlib/crc32_simd.c

Peter Kasting

unread,
Jan 5, 2025, 9:29:01 PMJan 5
to Adenilson Cavalcanti, Chris Blume, Adam Rice, Daniel Cheng, cxx
I would love to use a single "best" impl everywhere. I tend to assume that Abseil's impl is high quality and maintained for us unless told otherwise, but I'm happy with whatever decision is good. My role here is just to drive consensus :)

In this case I initially proposed replacing the third_party impl with the abseil one. Maybe we should replace the chromium one too, or should replace the third_party one with the chromium one and leave Abseil's banned. 

I assume all these are crc32c and not crc32c, but I could be confused. 

PK

Peter Kasting

unread,
Jan 5, 2025, 9:43:30 PMJan 5
to Adenilson Cavalcanti, Chris Blume, Adam Rice, Daniel Cheng, cxx
... And not crc32*, thanks phone

John Admanski

unread,
Jan 7, 2025, 5:53:46 PMJan 7
to cxx, Daniel Cheng, cxx, Peter Kasting
Re: the container guidance docs, I'm willing to take a crack at updating it to incorporate the abseil hash maps into it along the lines which Daniel is suggesting, if nobody else is working on that. It would includes dropping a bunch of the precise numbers the guidance currently cites for the reasons mentioned here.

Peter Kasting

unread,
Jan 7, 2025, 5:55:01 PMJan 7
to John Admanski, cxx, Daniel Cheng
On Tue, Jan 7, 2025 at 9:53 AM John Admanski <jadm...@chromium.org> wrote:
Re: the container guidance docs, I'm willing to take a crack at updating it to incorporate the abseil hash maps into it along the lines which Daniel is suggesting, if nobody else is working on that.

SG, go for it

PK 

Peter Kasting

unread,
Jan 13, 2025, 12:17:29 PMJan 13
to cxx, Peter Kasting, Chris Blume, Adam Rice, Daniel Cheng, cxx, Adenilson Cavalcanti
Adenilson, did you have further guidance on the best route for the Abseil impls, given the context below? Or can I provide more clarity?

PK

Peter Kasting

unread,
Jan 28, 2025, 8:31:47 PMJan 28
to cxx, Peter Kasting, Chris Blume, Adam Rice, Daniel Cheng, cxx, Adenilson Cavalcanti
Hearing nothing, here's the status --
  • Containers have just been allowed since there was no opposition there. Guidance is updated.
  • AnyInvocable and Logging will be banned. I'll leave notes or a bug on the logging side.
  • I think third_party/crc32c could get replaced by the Abseil one, and this wouldn't affect zlib since it is not crc32. It would be nice if someone wanted to run Abseil's benchmark over both. Any volunteers?
PK

Daniel Cheng

unread,
Jan 28, 2025, 8:33:46 PMJan 28
to Peter Kasting, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti
On Tue, 28 Jan 2025 at 12:31, Peter Kasting <pkas...@chromium.org> wrote:
Hearing nothing, here's the status --
  • Containers have just been allowed since there was no opposition there. Guidance is updated.

To be clear, this is not all containers, right?: this is just the hash set/map types.

Daniel 

Peter Kasting

unread,
Jan 28, 2025, 8:48:13 PMJan 28
to cxx, Daniel Cheng, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti, Peter Kasting
On Tuesday, January 28, 2025 at 12:33:46 PM UTC-8 Daniel Cheng wrote:
On Tue, 28 Jan 2025 at 12:31, Peter Kasting <pkas...@chromium.org> wrote:
Hearing nothing, here's the status --
  • Containers have just been allowed since there was no opposition there. Guidance is updated.

To be clear, this is not all containers, right?: this is just the hash set/map types.

It's whatever that CL is that got landed, that you reviewed. I assumed it put us in the state we wanted to be in. Do we need further guidance tweaks?

PK 

Daniel Cheng

unread,
Jan 28, 2025, 9:12:25 PMJan 28
to Peter Kasting, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti
No, that CL is fine. It's just that when I read the summary here, I got the impression that we were allowing all absl containers, hence the clarification question on my part.

Daniel


PK 

John Admanski

unread,
Jan 28, 2025, 11:00:59 PMJan 28
to cxx, Daniel Cheng, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti, Peter Kasting
One thing that was not included in any of the hash map container changes was any guidance on hashing. I couldn't really see a good place to just slot it in, so I chose to omit it rather than complicating the changes even more.

I don't know if it would make sense to suggest if you should make types hashable by specializing std::hash or by implementing AbslHashValue (or an even stricter stance than just suggesting).

Dan Murphy

unread,
Apr 23, 2025, 7:01:44 PMApr 23
to Daniel Cheng, Peter Kasting, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti
Just to ask - where is this guidance for absl containers documented? I see in the base/containers/README.md... but nothing yet uses absl in chrome/, so when I asked for this in this change it needed a DEPS change.... should this be "+absl/container"?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrmv0-w%2BkZs%3Db5s6G-2Gmaae3KoAGbLxRvJ-LKgF%3DmykA%40mail.gmail.com.

John Admanski

unread,
Apr 23, 2025, 7:49:16 PMApr 23
to cxx, dmu...@google.com, Peter Kasting, cxx, Chris Blume, Adam Rice, Adenilson Cavalcanti, Daniel Cheng
The base containers readme is the guidance that I updated. I also sent a change to update DEPS files and the style guide, but only DEPS files that already referenced absl/container.

As for the containers being used, I did make a bunch of changes afterwards to use them in more places...but that was all chromeos code, not chrome.

Reply all
Reply to author
Forward
0 new messages