nikic added a comment.

In D77621#2000237 <https://reviews.llvm.org/D77621#2000237>, @nikic wrote:

> > Perhaps you could move the value computation into a constexpr variable & 
> > just return that as needed. (could be a static local constexpr, I guess - 
> > to avoid the issues around linkage of constexpr member variables)
>
> The use of a function rather than a static constexpr for SizeTypeMax() was my 
> first thought as well. It seems pretty weird to me, but maybe it's enough to 
> fall one the wrong side of some inlining heuristic.
>
> The only other thing that comes to mind is that grow_pod() moved into the 
> header, which might have negative effects. It should be possible to avoid 
> that by providing explicit template instantiations for uint32_t and uintptr_t 
> in the cpp file.
>
> I'll try to figure out what the cause is, but might take me a few days.


I've tried those two things: results 
<http://llvm-compile-time-tracker.com/?config=O3&stat=instructions&branch=nikic%2Fperf%2Fsmallvector-size>.
 From the bottom, the first commit is a rebased version of the original change, 
the second one makes `SizeTypeMax` a constant instead of a function and the 
last one moves `grow_pod` back into the C++ file (I forgot to replace the 
UINT32_MAX references in grow_pod, but I don't think it has an impact on the 
conclusion). The first change is a `+0.75%` regression, the second is neutral 
and the last one is a `-0.70%` improvement, the remaining difference is likely 
noise. So it looks like the move of `grow_pod` into the header was the culprit.

What is rather peculiar is that the picture is similar for the max-rss numbers 
<http://llvm-compile-time-tracker.com/?config=O3&stat=max-rss&branch=nikic%2Fperf%2Fsmallvector-size>.
 I believe this is because max-rss is also influenced by the size of the clang 
binary itself, and apparently the move of grow_pod into the header increased it 
a lot. (I should probably collect clang binary size to make this easy to 
verify.) That means that there doesn't seem to be much of an increase in terms 
of actually allocated heap memory due to this change.

Taking the max-rss numbers 
<http://llvm-compile-time-tracker.com/compare.php?from=367229e100eca714276253bf95a0dd3d084a9624&to=a583ddc0a3ec034165d2762f791f5a88acc55a17&stat=max-rss>
 across all three commits, the only part where memory usage increases 
non-trivially is the LTO `-g` link step, by about ~1%. Possibly some debuginfo 
related stuff uses `SmallVector<char>`.

So tl;dr looks like as long as we keep `grow_pod` outside the header file, this 
change seems to be approximately free in terms of compile-time and memory usage 
both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to