[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);
+  }
----------------
dblaikie wrote:
> 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.
I also posted D131082 that helps a bit - gets us back under our 1% growth threshold we track, if that's of interest to upstream?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146