This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] replaces `__compressed_pair` with `[[no_unique_address]]` in `unique_ptr`
Needs RevisionPublic

Authored by cjdb on Jun 28 2023, 10:44 AM.

Details

Reviewers
EricWF
ldionne
philnik
Mordante
Group Reviewers
Restricted Project
Summary

__compressed_pair is a pre-[[no_unique_address]] way to ensure that
empty classes don't unnecessarily increase object sizes. It also
unfortunately bloats program sizes in debug builds due to added program
information. This commit replaces __compressed_pair with the
attribute, which reduces debug build sizes.

Below is a size benchmark of Clang built in debug mode with and without
__compressed_pair, using bloaty.

   FILE SIZE        VM SIZE
--------------  --------------
 +0.0% +1.94Ki  +0.0% +1.94Ki    .rodata
 +0.0%    +248  +0.0%    +248    .dynstr
 +0.2%     +28  [ = ]       0    .debug_loclists
 -0.5%      -1  [ = ]       0    .debug_aranges
 [DEL]     -56  [DEL]     -56    [LOAD #2 [R]]
 -0.2% -12.5Ki  [ = ]       0    .debug_abbrev
 -1.3%  -176Ki  [ = ]       0    .debug_rnglists
 -2.5%  -207Ki  -2.5%  -207Ki    .eh_frame_hdr
 -1.2%  -409Ki  [ = ]       0    .debug_addr
 -0.8%  -523Ki  [ = ]       0    .debug_str_offsets
 -2.5%  -622Ki  [ = ]       0    .symtab
 -2.5%  -830Ki  -2.5%  -830Ki    .eh_frame
 -0.7% -1.06Mi  -0.7% -1.06Mi    .text
 -1.0% -1.51Mi  [ = ]       0    .debug_line
 -3.2% -4.24Mi  [ = ]       0    .strtab
 -1.5% -5.99Mi  [ = ]       0    .debug_info
 -3.8% -10.3Mi  [ = ]       0    .debug_str
 -1.9% -25.8Mi  -0.9% -2.07Mi    TOTAL

Diff Detail

Event Timeline

cjdb created this revision.Jun 28 2023, 10:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 10:44 AM
cjdb requested review of this revision.Jun 28 2023, 10:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 10:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Jun 28 2023, 11:46 AM
philnik added a subscriber: philnik.

As-is this is an ABI break. Your unique_ptr<int> is currently 16 bytes large: https://godbolt.org/z/9xWPjTzK3. A lot of the reduced symbols is probably the avoidance of __compressed_pair right now. That is most likely possible to do in an ABI-stable way, but we need no_unique_address to work on windows for that to make sense. I'm also not convinced that duplicating the entire class is a good idea in terms of maintainability.

This revision now requires changes to proceed.Jun 28 2023, 11:46 AM

A lot of the reduced symbols is probably the avoidance of __compressed_pair right now.

On that note - spun off this discussion: https://discourse.llvm.org/t/reducing-overhead-of-compressed-pair/71660

A lot of the reduced symbols is probably the avoidance of __compressed_pair right now.

On that note - spun off this discussion: https://discourse.llvm.org/t/reducing-overhead-of-compressed-pair/71660

Indeed. compressed pairs symbols have been a pain in our ass forever. That's a problem worth tackling , but I'm not sure it's trivial to do that.
As Philinik say, no_unique_address is a bit of a problem. But I disagree that we need it on Windows in order proceed with changes.

IMHO our user base on Windows is so small that pessimizing Windows to optimize for the masses makes sense.

ldionne added inline comments.Jun 29 2023, 7:56 AM
libcxx/include/__memory/unique_ptr.h
713–715

I believe that makes us non-conforming? get_deleter() returns a reference to that, so different deleters need to have different addresses. It's a bit pedantic for sure, but this still seems like not what the standard intended us to implement unique_ptr as.

If we get the same (or almost the same) benefits from simply avoiding __compressed_pair, I would personally prefer that approach.

cjdb added inline comments.Jun 29 2023, 9:50 AM
libcxx/include/__memory/unique_ptr.h
713–715

The wording says that it needs to return a reference: it doesn't say that it needs to return references to unique objects. Given that this is also for a stateless allocator, I don't think it's non-conforming at all.

Note that I'm pretty committed to trying to get rid of __compressed_pair, since that improves the situation for everyone (but happy to debate the conformance matter above, here or elsewhere).

cjdb updated this revision to Diff 535954.Jun 29 2023, 12:20 PM
cjdb retitled this revision from [libc++] explicitly specialises `unique_ptr` when using `default_delete` to [libcxx] replaces `__compressed_pair` with `[[no_unique_address]]` in `unique_ptr`.
cjdb edited the summary of this revision. (Show Details)

updates patch to be about removing __compressed_pair instead of specialising the default_delete case

cjdb updated this revision to Diff 535964.Jun 29 2023, 12:55 PM
cjdb edited the summary of this revision. (Show Details)

slight update to the commit message

EricWF added a comment.Aug 3 2023, 1:50 PM

It's my understanding that the clang-cl changes to support [[no_unique_address]] are coming down the pipe, and will hopefully be here within the quarter.
I think we should wait until that lands, and then proceed carefully.

There's still some potentially breaking ABI changes like final but I'm open to erroring on that or supporting it minimally.

libcxx/include/__memory/unique_ptr.h
511–512

Shouldn't this follow the swap protocol using std::swap.

Mordante requested changes to this revision.Sep 4 2023, 11:49 AM
Mordante added a subscriber: Mordante.

It's my understanding that the clang-cl changes to support [[no_unique_address]] are coming down the pipe, and will hopefully be here within the quarter.

@EricWF do you have information when that clang-cl patch will land? We're moving to GitHub PRs when the patch does not land soon it might be best to abandon this patch and open a GitHub PR instead. @cjdb if you open a PR can you make sure you address all open comments on this patch?

This revision now requires changes to proceed.Sep 4 2023, 11:49 AM
akhuang added a subscriber: akhuang.Sep 7 2023, 2:23 PM

I'm working on a clang-cl no_unique_address patch here: https://github.com/llvm/llvm-project/pull/65675