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

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

In D90884#2377570 <https://reviews.llvm.org/D90884#2377570>, @dexonsmith wrote:

> In D90884#2377537 <https://reviews.llvm.org/D90884#2377537>, @rnk wrote:
>
>> In D90884#2377497 <https://reviews.llvm.org/D90884#2377497>, @mehdi_amini wrote:
>>
>>> Seems like it'll always have a default payload equivalent of 6 pointers? Maybe you can document this? Also that makes the vector 72B, which isn't cache line aligned, should we make it 64 or 128 instead?
>>
>> Right, the header is usually two pointers big (64-bit), so 3x the header is 72. Maybe keeping the size under 128 makes more sense.
>
> I find the multiple of header-size not so easy to reason about, since the header can be any of 12B, 16B, or 24B. I think I slightly prefer a limit like `<= 64` or `<= 8 * sizeof(void*)`.

Yeah, it's becoming clear that that's too hard to reason about. I propose that we guarantee that sizeof(SmallVector<T>) <= 8 * sizeof(void*). On 64bit platforms, gives us 4 pointers, two std::strings.

>> FWIW, I'm in favor of a conservative, low default size here. LLVM developers tend to use SmallVector for everything, even when it's inappropriate (many heap cases).
>
> Agreed; I've come around over time. I wonder about defaulting to `SmallVector<T, 0>`, getting POD optimization / compressed header / `SmallVectorImpl` interop, but no actual small storage unless you ask for it (I think this was your suggestion in the thread linked above).

I do think we need some small storage, as an overwhelming number of use cases really do have that intent (at least all of the ones that I have written in recent memory). I think this default protects us from some of the more pathological cases like SmallVector<SmallVector<T>> which, without the max-absolute-size default, just lead to an exponential blowup per "layer" of smallvector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90884