Page MenuHomePhabricator

In the libc++ unstable ABI, use [[no_unique_address]] instead of __compressed_pair when available.
Needs ReviewPublic

Authored by rsmith on Jun 24 2019, 4:13 PM.

Details

Summary

Adds a new __config macro _LIBCPP_USE_NO_UNIQUE_ADDRESS that specifies when to
use [[no_unique_address]] (instead of __compressed_pair or similar layout
hacks). This macro is defined only when the unstable ABI is enabled and
[[no_unique_address]] is available.

This reduces the number of template instantiations, compile-time cost, size of
debug information, and number of copies performed when using various parts of
libc++. In one source file, we saw a 4% decrease in object file size from
switching std::function to use [[no_unique_address]] instead of
__compressed_pair stemming primarily from reduced debug info size.

This patch does not intend to provide any ABI compatibility between the new
mode and old modes (or between two different compilers with the unstable ABI
enabled, where one supports [[no_unique_address]] and the other does not).
If we want a hard guarantee that even the unstable ABI provides stability
between compilers, we'll need to provide user control over whether we apply
this optimization rather than doing it whenever possible in the unstable ABI.

Event Timeline

rsmith created this revision.Jun 24 2019, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 4:13 PM
rsmith edited the summary of this revision. (Show Details)Jun 24 2019, 4:14 PM
rsmith updated this revision to Diff 206322.Jun 24 2019, 4:17 PM
  • Revert vestigial changes to __compressed_pair.
rsmith updated this revision to Diff 206323.Jun 24 2019, 4:18 PM
  • Revert one more vestigial change.
Harbormaster completed remote builds in B33836: Diff 206323.

A couple of general comments; still working through all the bits.

libcxx/include/__config
1019

This should really go with the rest of the ABI flags up around line #70, and let's give it an ABI name like _LIBCPP_ABI_USE_NO_UNIQUE_ADDRESS

libcxx/include/__hash_table
3068

Shouldn't these be wrapped in a #ifdef _LIBCPP_USE_NO_UNIQUE_ADDRESS block?

rsmith updated this revision to Diff 206346.Jun 24 2019, 5:49 PM
rsmith marked an inline comment as done.
  • Address a couple of review comments from Marshall.
rsmith marked an inline comment as done.Jun 24 2019, 5:50 PM
rsmith updated this revision to Diff 206347.Jun 24 2019, 5:55 PM
  • Rename __hash_ macro to avoid name collision.

Thanks for working on this! __compressed_pair is a menace which causes serious template and code bloat.
And the code without it is much clearer and cleaner.

But...

I'm concerned by the number and size of the conditional compilation blocks this introduces.
In my time on libc++ conditional compilation blocks have been a frequent source of bugs; and I've been working to remove as many as I can.

I haven't figured out a cleaner way to implement this.
Does anyone have ideas on how we might eat our cake and have it too?

libcxx/include/__hash_table
1042

I really don't like this #define approach. Perhaps we could create accessor functions instead?

ldionne requested changes to this revision.Jun 27 2019, 10:31 AM
ldionne added a subscriber: ldionne.

I like this, however I want to echo @EricWF 's concern about the conditional compilation this introduces. Do we get any benefit if we still use compressed_pair but switch THAT to use no_unique_address instead?

This revision now requires changes to proceed.Jun 27 2019, 10:31 AM
rsmith added a comment.EditedJun 27 2019, 12:56 PM

I like this, however I want to echo @EricWF 's concern about the conditional compilation this introduces. Do we get any benefit if we still use compressed_pair but switch THAT to use no_unique_address instead?

Some, but much much less. In my experiments, converting __alloc_func to use [[no_unique_address]] directly had four times as much impact as converting __compressed_pair to use [[no_unique_address]]. Also, probably minor, but we can't easily get rid of the redundant (eg) allocator move constructions unless we remove the extra layer of indirection.

rsmith updated this revision to Diff 206949.EditedJun 27 2019, 3:11 PM
rsmith marked 2 inline comments as done.
  • Replace #defines of field names in non-[[no_unique_address]] mode with accessor functions
libcxx/include/__hash_table
1042

OK. This will re-add some compilation overhead to the new mode that I was trying to eliminate, but it should be no more than we pay in the old mode. I certainly don't have any measurements to justify this ugliness.

phosek added a subscriber: phosek.Jul 11 2019, 6:49 PM

Ping. Is there any interest in pursuing this, or is the preprocessor complexity too great?

Ping. Is there any interest in pursuing this, or is the preprocessor complexity too great?

Ping.

thakis added inline comments.
libcxx/include/__config
121

Can we make this defined(_LIBCPP_ABI_UNSTABLE) && !defined(_LIBCPP_ABI_USE_NO_UNIQUE_ADDRESS?

We have a natviz file to tell msvc's debugger how to display libc++ types, and I don't know how well codeview can describe no_unique_address. Furthermore, the natviz files currently explicitly mention the compressed pairs.

So it'd be good if we could opt out of this for a while on windows after it lands, to prepare the visualizers.

IMO, this is a good idea, and should be committed evendespite the addition of ifdefs in the implementation.

We'd definitely want to do this in a brand new ABI, and there's not really any better way to accomplish it now.

libcxx/include/__config
122

IMO, the ABI shouldn't change based on a compiler feature-test, even in unstable-abi mode.

But I don't see why it'd be a problem to just enable it by default with _LIBCPP_ABI_UNSTABLE, especially if, per the request above, there's a way to explicitly disable it again.

Then, if _LIBCPP_ABI_USE_NO_UNIQUE_ADDRESS is enabled, it should just emit an #error when the compiler fails to support the attribute.

libcxx/include/memory
2611

ifdef in the middle of an expression isn't worth it. Just repeat the rest of the statement.