[PATCH] D87326: [ADT] Fix reference invalidation when self referencing a SmallVector

Tue Sep 15 03:20:24 PDT 2020

njames93 added a comment.

In D87326#2273355 <https://reviews.llvm.org/D87326#2273355>, @nikic wrote:

> Tested the newest version of this patch and I'm still seeing **massive** regressions, even larger than before.
>
> Compile-time (instructions retired): https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=instructions This is now a 1.4% geomean regression at O3 <https://reviews.llvm.org/owners/package/3/>, with 2% at O3 <https://reviews.llvm.org/owners/package/3/>, with tramp3d-v4 hitting 3%.
>
> Max RSS: https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=max-rss This is now a 1.6% geomean regression at O3 <https://reviews.llvm.org/owners/package/3/>, with 2.1% at O0.
>
> Clang text size goes from 80560905 to 82179908, a 2% regression (non-LTO build using GCC 9.3).

I'm still seeing a different picture with clang-10 as the host compiler, I'm only targetting x86 which explains why my binaries are so much smaller than yours.

  orig-o3  54404996
  this-03  54046228
  orig-lto 69549814
  this-lto 65981798

The most likely reason for a larger binary in GCC is in the original file, the calls to grow weren't inlined. Now, most of that code for using a new buffer is inlined. Clang may handle things a little differently.
Maybe I could define the swap buffer methods out of line to dissuade inlining.

================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:478-480
+  EXPECT_EQ(Constructable::getNumCopyAssignmentCalls() +
+                Constructable::getNumCopyConstructorCalls(),
+            2);
----------------
dblaikie wrote:
> Worth testing for the specific amounts separately, or does it vary over the test variations?
If the container grows there will be 2 copy constructors called, if it doesn't, there is one copy constructor and one copy assign. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326