Switching from base::WrapUnique() to absl::WrapUnique()

[Lei Zhang]

These two utility functions [1] [2] are basically identical, but the
absl version comes with more static_asserts. Should Chromium switch to
the absl version, now that absl has been default allowed for about a
year?

[1] https://chromium.googlesource.com/chromium/src/+/eb5318901b/base/memory/ptr_util.h#15
[2] https://chromium.googlesource.com/chromium/src/+/eb5318901b/third_party/abseil-cpp/absl/memory/memory.h#71

[Peter Boström]

Those are very good asserts. +1

--
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 on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACSHbcS3qLv5Mbhx3hsEdNM4%2BFErJ%2BeNc05rz0bz6c0Scs9EEw%40mail.gmail.com.

[Peter Kasting]

More asserts good, fewer ways of doing the same thing good. +1

PK

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

[danakj]

Possibly hot take but we currently have one way of doing the same thing, since we use the base one. We could also just keep it and implement it on top of absl's. Then if we want to do something else later it's easy. Thoughts?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFATNres8Z%2BsJygpLwUMD0nzorSrpsLzdzJCDHJz04F%3DYg%40mail.gmail.com.

[Peter Boström]

Rolling our own because we may want to roll our own in the future seems too weak an argument to me (switch replace should be straightforward in both directions bar iwyu). Do you have any specific things such as magic with raw_ptr in mind or is this a preference not to use absl in general?

[danakj]

On Thu, Feb 8, 2024 at 10:31 AM Peter Boström <pb...@chromium.org> wrote:

Rolling our own because we may want to roll our own in the future seems too weak an argument to me (switch replace should be straightforward in both directions bar iwyu). Do you have any specific things such as magic with raw_ptr in mind or is this a preference not to use absl in general?

I see the weeks and weeks of work to do a trivial transform of absl::optional to std::optional unfolding before me, and I wonder how we avoid making that situation again.

[Alexei Svitkine]

I don't see the benefit of switching it from being "base::" prefixed with "absl::" prefixed, since it's a bunch of churn CLs.

If we just want the asserts, we can make the "base::" prefixed one be an alias for the absl impl, without needing to change all the calls.

(I guess in contrast, I do see the switch to "std::" prefix a bit different, since at least the std:: one is one that C++ devs will be familiar with and will stick forever, whereas e.g. whatever is an absl we may need to switch to std:: later anyway if an equivalent one gets added there.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQxAhYjSM6gS-_JiHuhAd6h_pvOdkJrDzD9uWLj8hM%2B6Q%40mail.gmail.com.

[Avi Drissman]

For me the benefit of moving from base:: or absl:: to the std:: version is that as a user it's a lot easier to include the correct header. IMO switching from base:: to absl:: is a step backwards, for which using WrapUnique will now be a pain as I'll have to hunt down a long and complicated include file path every time.

(I was cheering when we moved optional to std:: because I literally had to codesearch each time to include the correct absl header.)

Avi

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKFU2SAcNOCH6tD79Hz3nbyJx4Lg-otGf7cNAWR6ZoMF1o9pVA%40mail.gmail.com.

[Peter Boström]

How about let's start with using `using` and probably for now not spend too much time on it? That way we won't functionally diverge without intending to and we get the asserts without copying?

[Peter Kasting]

Switching costs are not a very compelling argument to me. This particular switch is trivially scriptable and can be landed in a single large cl with OO+1.

PK

[danakj]

On Thu, Feb 8, 2024 at 2:53 PM Peter Kasting <pkas...@google.com> wrote:

Switching costs are not a very compelling argument to me. This particular switch is trivially scriptable and can be landed in a single large cl with OO+1.

You'd think so but so should optional, it's an alias, but it's showing me "trivial" is not.

[Peter Kasting]

What cost are you specifically worried about with "a bunch of churn CLs"? Author time? Reviewer time? Inbox clutter? Degraded blame? It behooves us to understand what developers find painful so we can fix it, because minimizing transform costs is critical to being able to rapidly and repeatedly make small codebase-wide improvements in the future.

PK

[danakj]

Author time and reviewer time.

[Peter Kasting]

This is a fair complaint. It would be nice to have better clangd/auto include stuff like Cider-V has.

In fairness, though, WrapUnique should be a fairly unusual thing. (I suspect a lot of our existing usage is unnecessary.) I use it orders of magnitude less often than optional<>.

PK

On Thu, Feb 8, 2024, 9:01 AM Avi Drissman <a...@google.com> wrote:

[Peter Kasting]

What has made the CLs non trivial? I have not been involved in writing them, so maybe there's an obvious piece I missed.

Is there a specific reason we can't do optional<> in fewer, larger pieces? (One cl might be a bridge too far in terms of merge conflicts, since optional is such a common type.)

PK

[Peter Kasting]

Agree that WrapUnique is not worth a significant amount of either.

Let's say the total combined author+reviewer time for this could be reduced to under one hour. (I actually believe this is feasible.) Would you still be opposed?

PK

[danakj]

On Thu, Feb 8, 2024 at 3:02 PM Peter Kasting <pkas...@google.com> wrote:

> What has made the CLs non trivial? I have not been involved in writing them, so maybe there's an obvious piece I missed.

There are many enormous CLs. Reviewers vary on how fast they get to them. There's various amounts of feedback on missing includes that got missed or whatever little things. Each CL may have conflicts before it lands. There are multiple rounds of review, which then gets an OO sometimes to make up the last changes. It's a slog, basically.

Very easy in a world where you're the only committer, very much not in Chromium.

Agree that WrapUnique is not worth a significant amount of either.

Let's say the total combined author+reviewer time for this could be reduced to under one hour. (I actually believe this is feasible.) Would you still be opposed?

Weakly because the include issue avi@ mentioned, but not based on work/effort then.

[Peter Kasting]

Interesting. It seems like if reviewers have any feedback at all, either

(a) something went wrong with scripting the conversion -- how could we be missing #includes (at least, worse than we already were)?

(b) reviewers are asking for incidental "while here" cleanups on a giant lsc, which is inappropriate

Is this a, b, or did I miss one?

PK

[danakj]

On Thu, Feb 8, 2024 at 3:35 PM Peter Kasting <pkas...@google.com> wrote:

Interesting. It seems like if reviewers have any feedback at all, either

(a) something went wrong with scripting the conversion -- how could we be missing #includes (at least, worse than we already were)?

(b) reviewers are asking for incidental "while here" cleanups on a giant lsc, which is inappropriate

Is this a, b, or did I miss one?

Both from what I see. In an ideal world these are just stamped but not always, and there's latency involved, and the includes are already not correct.

Maybe it's worth sitting down with arthursonzogni and going through how the process was when he is done. But I have learnt that "trivial" changes take a lot of time and are thus quite expensive.

[Peter Kasting]

Hmm. It seems like as long as things compile, and we don't erroneously omit includes that had previously been present, reviewers should stamp without any requests for changes. The normal route to do this is via LSC with one or more global approvers who understand that, so we don't need to remind individual reviewers of it.

Is optional going through the lsc process? If not, that seems like the missing piece here. We have that process precisely for this case; it can cut total time by 95+%.

PK

[Arthur Sonzogni]

I think absl::optional was a bit special due to its size.

• absl::optional was 150k line change (20k files) (with the #includes and the absl::nullopt)
• base::WrapUnique is going to be ~4k line change (1.9k files)

base::WrapUnique might be more manageable?

If you are curious, here are the main problems I encountered  with absl::optional:

Merge conflict:

---------------------

In Chrome, there is a commit every 4-5 minutes on average:

• Patches of 20k files have a half life of about 6 minutes before getting a merge conflict. So you have to split.
• Patches of 1k files have a half life of about 2 hours. That's just enough time for getting a reviewer to +1.

Often you don't get a review on time, and reapplying the patch causes losing +1.
So, you end up relying on an helpful base Owner with Owner-Override (Thanks Dana!), and/or you retry the next day.

IWYU:

--------

Many parts of Chrome relies on transitive includes. If you split a 20k patch in 20 parts, you get many new errors.

You have to land the patches in a "good" sequence and follow the dependencies. If you don't you have to fix them yourself when it is manageable.

Note: there are also several transitive headers from abseil that ended up being missing in some files, like <string>.

<windows> header.

----------------------------

The clang-format `IncludeBlocks: Regroup` option was very useful to reorder the #includes correctly.
Unfortunately several headers from Microsoft are relying on `#include <window>` being included before causing compile errors.

So you have to introduce several `clang-format off` to prevent reordering those parts. Example.

Arthur @arthursonzogni

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRWN1i2H%2BUsQTFe8FAkqJn5OH_dj_Pc3%3D7Wm%3DCPKu7Pcw%40mail.gmail.com.

[Peter Kasting]

The transitive includes issue is indeed annoying. I'm surprised it bit us much here assuming ABSL_USES_STD_OPTIONAL is true, though. That does pull in "absl/utility/utility.h" unnecessarily, which might be the source of our problems. We probably should try and fix that upstream.

In the case of base::WrapUnique, the header that defines that only includes <memory>, which is also in the header that defines absl::WrapUnique, so this can't be an issue.

For reviewer latency -- are you using an LSC global approver? That way you can fire a stream of large CLs, they can stamp+OO quickly, and you can land. If not, I strongly advise going this route.

For include reordering -- the windows.h issue is also annoying. We've addressed this two ways in the past: (1) preferring base/win/windows_types.h whenever possible (which needs no special ordering), and (2) placing windows.h in its own newline-delimited block otherwise, which prevents clang-format from trying to reorder. I imagine we have places that violate those. We could probably do a separate pass to find and fix these by just running a global "fix include order" pass and seeing what it turns up. I would be happy to help do that.

PK

[Avi Drissman]

On Thu, Feb 8, 2024 at 4:21 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(2) placing windows.h in its own newline-delimited block otherwise, which prevents clang-format from trying to reorder.

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDrOvJ3Jz2y2vN7MCZaRi4mZuVVyrzT9JYGSDmDv%2ByCxQ%40mail.gmail.com.

[danakj]

On Thu, Feb 8, 2024 at 4:26 PM Avi Drissman <a...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 4:21 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(2) placing windows.h in its own newline-delimited block otherwise, which prevents clang-format from trying to reorder.

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

Really this should be configurable in clang-format in some way..

[Arthur Sonzogni]

The transitive includes issue is indeed annoying. I'm surprised it bit us much here assuming ABSL_USES_STD_OPTIONAL is true, though. That does pull in "absl/utility/utility.h" unnecessarily, which might be the source of our problems. We probably should try and fix that upstream.

Most are about unrelated files missing the definition of absl::optional, because they only got transitively <optional> instead of abseil.

There was also the definition of string, or variants that many are forgetting.

For reviewer latency -- are you using an LSC global approver? That way you can fire a stream of large CLs, they can stamp+OO quickly, and you can land. If not, I strongly advise going this route.

The main difficulty was not being able to land every patch in parallel, because of the missing transitive headers.

In reality, most reviews were very easy, except a single one.

We used OO 20% of the patches (6/29 patches)

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

Yes, I was also very happy when I saw it. It landed, but then they decided to revert it. I think this was because of <window.h> header.

Arthur @arthursonzogni

[Arthur Sonzogni]

The transitive includes issue is indeed annoying. I'm surprised it bit us much here assuming ABSL_USES_STD_OPTIONAL is true, though. That does pull in "absl/utility/utility.h" unnecessarily, which might be the source of our problems. We probably should try and fix that upstream.

Most are about unrelated files missing the definition of absl::optional, because they only got transitively <optional> instead of abseil.

There was also the definition of string, or variants that many are forgetting.

For reviewer latency -- are you using an LSC global approver? That way you can fire a stream of large CLs, they can stamp+OO quickly, and you can land. If not, I strongly advise going this route.

In reality, every review was very easy (except one). We used OO 20% of the patches (6/29 patches)

The real difficulty was not being able to land every patch in parallel, because of the missing transitive headers. Doing them one by one, and finding the right order was the difficult part.

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

Yes, I was also very happy when I saw it!

It has been reverted, because of <windows.h> header.

I'm looking forward to a reland!

Arthur @arthursonzogni

[Alexei Svitkine]

I wonder if we should have a mechanism to land CLs atomically. Like some time window where we close the tree and let only such approved changes in that could be scheduled on-demand during off-peak hours.

(I guess it still would cause merge conflicts for other people's CLs that are in flight though.)

[Daniel Cheng]

On Thu, 8 Feb 2024 at 13:33, danakj <dan...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 4:26 PM Avi Drissman <a...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 4:21 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(2) placing windows.h in its own newline-delimited block otherwise, which prevents clang-format from trying to reorder.

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

Really this should be configurable in clang-format in some way..

Also see https://crbug.com/40641829.

In the past, I tried fixing this using include categories (https://clang.llvm.org/docs/ClangFormatStyleOptions.html#includecategories), but this didn't work well because clang refused to re-order across headers delimited by newline blocks.

If clang-format is moving headers into between blocks now, maybe these directives will work again (though I'm told that they might heavily impact formatting speed)?

Daniel

[Luciano Pacheco]

I'm new to the cxx, but how worried are we about breaking usage of base:: outside Chromium?

For example ChromeOS uses base::WrapUnique from libchrome. If we end up fully removing base::WrapUnique those would break.

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

[Peter Boström]

Libchrome is a quasi-supported downstream of base, generally they may patch in things (and locally revert) to keep stuff working until downstream is ready to migrate off. We normally do not prevent changes in base/ due to libchrome transitive dependencies (and they are not part of our CQ). Others have more detailed info, but we don't "really" support out-of-tree base, which is why v8 carries its own base (heavily influenced by / copied from //base), for instance.

[Peter Kasting]

Dumb idea: create a top level "absl/" dir containing forwarding headers to the third_party ones, only for the allowed headers. Then the DEPS rules just become "+absl/", and we chop "third_party/abseil-cpp/" off all absl includes in first-party code.

Thoughts?

PK

[Marijn Kruisselbrink]

On Thu, Feb 8, 2024 at 3:01 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Dumb idea: create a top level "absl/" dir containing forwarding headers to the third_party ones, only for the allowed headers. Then the DEPS rules just become "+absl/", and we chop "third_party/abseil-cpp/" off all absl includes in first-party code.

Thoughts?

Previously discussed in https://groups.google.com/a/chromium.org/g/cxx/c/DwpCC68qmlg/m/QR_GIaAnBQAJ

I don't think anything really changed since that discussion in this area?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBpSvBXozZneanNbR9vL0iZ7OFXoiMHVP4z63qkZLzY-g%40mail.gmail.com.

[Joe Mason]

It seems like there's a lot to discuss about rewriting to absl::WrapUnique, and LSC's and absl in general. In the meantime: is there any reason NOT to replace ptr_util.h with "using WrapUnique = absl::WrapUnique" while that discussion is going on?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BOSsVb%2BY9Z%2Bn0TSb8YejV4yqofvKf9QzUEAXD1updvizBsw3g%40mail.gmail.com.

[Peter Kasting]

I think what's changed is that

(a) we now have precedent for doing this frequently and pervasively, e.g raw_ptr.h

(b) we now allow iwyu pragma comments, so we can direct tools to which header is canonical

PK

[K. Moon]

Re: std adding something similar: Never say never, but this is unlikely because it's impossible to distinguish T* and T[].

The logical way to add this to the standard library is as a CTAD to the unique_ptr constructor, but this was rejected for precisely that reason. Adding a separate helper function doesn't really help that situation, as it's still a footgun.

Note that make_unique doesn't have this issue, because it's constructing the T or T[].

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

[K. Moon]

One other point: The types returned by the base and absl version are interoperable (that is, they're exactly the same). There's a certain amount of, "We shouldn't have two ways to do things" to it... but we could just allow both and let it work itself out over time.

[Lei Zhang]

I can certainly add that using statement.

I felt Avi's pain when we initially switched from base::optional to
absl::optional. and wished we kept base/optional.h as a forwarding
header. But over time, I got over it. As absl::optional is used in so
many places, it was very likely that some file I had open had the
#include I needed. So it was just a copy + paste most of the time.

[Lei Zhang]

I'm having second thoughts about including
third_party/abseil-cpp/absl/memory/memory.h, which also makes it
easier to use absl::ShareUniquePtr() and absl::WeakenPtr(), when
std::shared_ptr and std::weak_ptr are banned in first-party code. So
just adding some static_asserts to base::WrapUnique() for now.
https://crrev.com/c/5279160

[Peter Kasting]

On Thu, Feb 8, 2024 at 1:53 PM Daniel Cheng <dch...@chromium.org> wrote:

On Thu, 8 Feb 2024 at 13:33, danakj <dan...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 4:26 PM Avi Drissman <a...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 4:21 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(2) placing windows.h in its own newline-delimited block otherwise, which prevents clang-format from trying to reorder.

Is this still the case? I recall we tried to roll in a new version of clang-format that rebuilt the entire include section. (I was actually a big fan of it, except for the fact that it sometimes reordered <windows.h>.

Looks like it depends on the IncludeBlocks setting. IBS_Preserve only sorts within delimited blocks. IBS_Merge collapses everything into a single block. And IBS_Regroup collapses, then creates a new set of blocks based on the IncludeCategories.

In the past, I tried fixing this using include categories (https://clang.llvm.org/docs/ClangFormatStyleOptions.html#includecategories), but this didn't work well because clang refused to re-order across headers delimited by newline blocks.

Sounds like IncludeBlocks was set to IBS_Preserve. We almost certainly want to set it to IBS_Regroup and muck with the IncludeCategories until they Do The Right Thing Everywhere.

Per https://github.com/llvm/llvm-project/blob/b2ea04673b782f95ac9841f87df8bb5f7b561067/clang/lib/Format/Format.cpp#L1786, it looks like IBS_Preserve is still the upstream setting. Maybe we should have a bug somewhere on trying to change this?

PK

[Arthur Sonzogni]

FYI: Here is a pending patch from @Takuto Ikuta about using IncludeBlocks: Regroup ( bug 329138753)

The win32 api implicit include order dependencies was fixed by grouping them in their own separate category.

I guess it can be upstreamed if we end up happy with this configuration.

Arthur @arthursonzogni