[PATCH] D90884: [SmallVector] Add a default small size.

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

In D90884#2387054 <https://reviews.llvm.org/D90884#2387054>, @mehdi_amini wrote:

>> I think setting an inline storage size without profiling is often a premature optimization.
>
> I don't necessarily agree with this: I believe this is the kind of situation where you end up with flat profile where malloc shows up everywhere, and then we would spend a huge amount of resources in optimizing malloc() (like some company specialized in moving protos where optimizing memcpy is easier than avoiding the copy in the first place ;)).
> I remember seeing this in clang in the past with `strlen` taking a few percent on the profile, but it was really hard to fix it because of how widespread it is in the codebase: this is why idioms/patterns around APIs are important.
> (also it isn't only about the malloc time, but I suspect the caching behavior of using the stack for short lived value likely help).

Agreed on the flat profile. I think that tuning this must be done in aggregate, and having a useful default here is the key to enabling that. We can make the policy arbitrarily complex, like having a SmallVectorTraits or something, where e.g. Value* can specialize it to say something like "most vectors of Value* are of operands, which are usually at most 3 operands" (don't know if that's true btw).

> At the moment, I much prefer the current behavior and our usage of SmallVector to a "std::vector with implicit inlined storage sometimes, maybe, but you don't really know": when I read or review LLVM code I know what it is doing without the need for too much more context (I see it a bit similarly to our guideline on not abusing `auto` for readability).

If I write a std::vector on the stack in a patch, I think most reviewers will ask "why not SmallVector?" and even suggest using SmallVector by default. And then most people will just add a random smallsize and nobody cares. My favorite is 6.

I think that it's pretty clear that as a community we are using SmallVector as a vector replacement -- it's the default, and we should make it convenient to use, and make the default usage "just work" and avoid pitfalls (the main one being unexpectedly large sizeof).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90884