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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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:
> > > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146