[libcxx-commits] [PATCH] D128146: [libc++] Use uninitialized algorithms for vector

[David Blaikie via Phabricator via libcxx-commits]

dblaikie added inline comments.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:634
+  } else {
+    return std::move(__first1, __last1, __first2);
+  }
----------------
philnik wrote:
> dblaikie wrote:
> > philnik wrote:
> > > dblaikie wrote:
> > > > Ah, this is another source of debug info growth - the `std::move(iter, iter, iter)` implementation instantiates `std::pair` (so this change added an extra 2,000 instantiations of `std::pair` to a clang dbg build - and `std::pair` isn't especially light weight with the compressed pair types, all the members, etc.
> > > > 
> > > > Any chance the implementation of `std::move` could be changed to avoid using `std::pair` - while I realize that might involve some code duplication if the underlying helpers are used in a few places that want both parts of the paired result, it might still be worthwhile.
> > > > 
> > > > I'll look into making a prototype.
> > > > 
> > > > Oh, I didn't send this, and did the prototype... so results: It looks like the `__move_impl` doesn't actually use the pair result at all (the ranges-based move does use it, but the code isn't shared - maybe it was at some point) so I removed it, and that got the total `.dwp` change for this uninitialized patch + that move(iter, iter) patch be slightly negative:
> > > > ```
> > > >     FILE SIZE        VM SIZE
> > > >  --------------  --------------
> > > >   +0.2%  +571Ki  [ = ]       0    .debug_str.dwo
> > > >   +0.2% +26.3Ki  [ = ]       0    .debug_rnglists.dwo
> > > >   +0.0% +15.5Ki  [ = ]       0    .debug_str_offsets.dwo
> > > >   +0.3% +1.67Ki  [ = ]       0    .debug_loclists.dwo
> > > >   -0.2% -17.0Ki  [ = ]       0    .debug_abbrev.dwo
> > > >   -0.2%  -697Ki  [ = ]       0    .debug_info.dwo
> > > >   -0.0% -99.5Ki  [ = ]       0    TOTAL
> > > > ```
> > > > 
> > > > Net reduction of about 2k std::pair instantiations, rather than a net increase of about 2k with only the uninitialized patch.
> > > > 
> > > > though `__compressed_pair`/`__compressed_pair_elem` instances didn't change by much - *shrug*. 25% (3k down from 4k) fewer `make_pair` instantiations. Few other minor things of note.
> > > > 
> > > > the increases in `construct` (5933 -> 6229) and `reverse_iterator` (7827 -> 8786) are probably somewhat unavoidable/expected. (well, I understand the construct ones - oh, the reverse iterator ones might be for the destruction codepath that this patch mentions is a bugfix/intentionally added)
> > > > 
> > > > I'll send out the __move pair removal cleanup shortly.
> > > What do you mean with "the code isn't shared"? `__move` is called from `ranges::move` [here](https://github.com/llvm/llvm-project/blob/f81a209337bbcf527e0b4adfcfedb44d798ac91d/libcxx/include/__algorithm/ranges_move.h#L42). Maybe you looked at some in-between commit or missed the call?
> > Yeah, I didn't look closely enough. So this'd require some code duplication or conditionality - I'm open to ideas - I'll throw up a straw-man patch at least.
> Could you check whether applying D131198 helps?
After fixing up a few things (there were still a few mentions of `std::pair` in `move.h` and uses of `first` and `second instead of `__first_` and `__second_`, `make_pair` replaced with `{}`, but got the general idea) I ran that and got this comparison:
```
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.6% +1.48Mi  [ = ]       0    .debug_str.dwo
  +0.1%  +513Ki  [ = ]       0    .debug_info.dwo
  +0.2%  +126Ki  [ = ]       0    .debug_str_offsets.dwo
  +0.3% +34.5Ki  [ = ]       0    .debug_rnglists.dwo
  +0.3% +1.67Ki  [ = ]       0    .debug_loclists.dwo
  -0.2% -16.0Ki  [ = ]       0    .debug_abbrev.dwo
  +0.3% +2.13Mi  [ = ]       0    TOTAL
```
(this is the diff/growth in .dwp between building clang with two different compilers, one that includes the uninitialized patch + the __libcxx_pair_ (along with a bunch of other changes - whatever's changed between our release and testing-the-next-release compiler - I can get specific versions if it's useful, but it looks like most of the growth is due to this uninitialized patch) and one that includes neither)

Looking at the string count diffs - 2473 strings starting with "__libcpp_pair", 4133 -> 3309 instances of "make_pair"

So, it seems it does certainly lower the cost, but not as low as not using a pair here at all/possibly duplicating the code so there's a separate copy for range-based-move.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128146/new/

https://reviews.llvm.org/D128146