<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 16, 2020 at 2:12 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On Mon, Nov 16, 2020 at 1:55 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br>
> On Mon, Nov 16, 2020 at 12:55 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> I will say I'm not a huge fan of adding even more names for things in<br>
>> this fairly core/common use case (now we'll have even more vector<br>
>> names to pick from) - can we use default template arguments so we can<br>
>> write SmallVector<T> instead of SmallVector<T, N> and would that<br>
>> address some of the use cases proposed here?<br>
><br>
><br>
> I won't claim it is perfect, but the added names are a compromise over rounds of reviews with the folks in the revision. In particular I'm quite concerned that a default value for N on the SmallVector does not carry the intent the same way, and is too easy to miss in review (or while reading code).<br>
<br>
I'm not quite following here - what sort of problems do you anticipate<br>
by readers missing the default value for N?<br></blockquote><div><br></div><div>Reading code `SmallVector<SomeType>` does not express that it is *intentional* to leave of the value for N. It can easily be just forgotten, and it could easily be implicitly `0`, and as a reviewer (or code reader later) I don't know if this is was intentional or not. This is why I am quite opposed to "loosen" the current requirement on SmallVector: a different name for a different intent is better fitting to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> To me the drawbacks are outweighing the benefits too much.<br>
> Also, `SmallVector<LargeType>` would end-up with N==0 implicitly, without an easy way to figure it out that there is no actual inline storage while reading the code.<br>
<br>
Why would it be necessary to figure that out? If we generally feel the<br>
right default inline storage happens to be zero in that case and that<br>
SmallVector will pick a good default (including possibly 0) that seems<br>
like a good thing to me. (why would we call out zero specially, if we<br>
want to avoid calling out other values explicitly)<br></blockquote><div><br></div><div>I think this is an API that is "easy to misuse", I don't see any advantage to it while it has drawbacks: why would we do that?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> An alternative was to reserve the default to only "small object" so that N isn't zero, but there isn't a platform independent way of doing that and keep the code portable I believe. So `SVec<T>` is really saying: "I am willing to pay for some limite inline storage if possible but I don't have a N in mind".<br>
<br>
That quoted statement still sounds like it could encapsulate the<br>
possibility of 0 too, though. "limited inline storage if possible"<br>
could be "and the answer to that constraint is zero in this case - but<br>
if you happen to make the T smaller, it could become non-zero<br>
organically/without the need to touch this code" (as the N could vary<br>
organically between non-zero values as struct sizes change too)<br></blockquote><div><br></div><div>Yes the quoted statement says exactly that! This is why I don't like having this behavior on SmallVector: this is a different semantics / intent that the programmers have and this is something that I like being able to read differently. </div><div> SVec does not accept a `N`: if I read SVec<Type> there can't be a possible accidental omission of N here. It has to be a choice between `SmallVector<SomeType, 4>` and `SVec<SomeType>` but not `SmallVector<SomeType>`.</div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
> Finally the simple `llvm::Vector` case to replace `SmallVector<T, 0>` is because it is frequently preferable to `std::vector` but still isn't readable or immediately intuitive and so is rarely used in practice (see <a href="https://www.llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h" rel="noreferrer" target="_blank">https://www.llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h</a> for the documented points on N=0).<br>
<br>
I'm not sure llvm::Vector/Vec would necessarily make it more often<br>
used in practice. Maybe a bunch of cleanup and more specific wording<br>
in the style guide that would ban std::vector might help more there.<br>
<br>
Though I agree that it may be suitable to have a clear name for<br>
SmallVector<T, 0> since the "Small" is slightly misleading (though not<br>
entirely - it's important for the "SmallVectorImpl" benefits of<br>
SmallVector - so renaming it to Vector and still using it via<br>
SmallVectorImpl<T>& might also be confusing).<br>
<br>
>> Got any sense of the total value here? Major savings to be had?<br>
>><br>
>> (if SmallVector<T> can do what your proposed SVec<T> does, that leaves<br>
>> the Vec<T> - could you expound on the benefits of SmallVector<T, 0><br>
>> over std::vector<T>? I guess the SmallVectorImpl generic algorithm<br>
>> opportunities? Though that's rarely needed compared to ArrayRef.)<br>
>> If SmallVector<T> would suffice then maybe Vec<T> could be<br>
>> ZeroSmallVector<T>? Not sure.<br>
><br>
><br>
> ZeroSmallVector<T> does not really address your "more vector names to pick from" concerns,<br>
<br>
Somewhat - if SmallVector<T> can be used instead of SVec<T> then we<br>
have one fewer name and less convention to base the Vec<T> on (since<br>
it won't have the SVec<T> sibling), so picking a name closer to the<br>
existing SmallVector might be more viable/helpful.<br>
<br>
> and it is longer than `SmallVector<T, 0>`: shouldn't we aim for the "default case" to be the easiest to reach / most intuitive to pick? `llvm::Vec` looks like "just a vector".<br>
<br>
Somewhat - but Vec is an abbreviation that isn't really used otherwise<br>
(if we consider SVec as well, I'd still push back on the introduction<br>
of the abbreviation for both cases)</blockquote><div><br></div><div>One aspect of the naming is to avoid one not being a prefix of the other (IDE completion, etc.) or them being too close to differentiate.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> and llvm::Vector would be only a<br>
case separation away form a standard name (when considering the<br>
unqualified name - which is likely to come up a fair bit, as we'll see<br>
"Vector" used unqualified a lot).<br></blockquote><div><br></div><div>Note: this is why we converged with llvm::Vec and not llvm::Vector in the revision</div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
I wouldn't entirely object to SmallVector<T> getting a smart default<br>
buffer size, and llvm::Vector being an alias for SmallVector<T, 0> - I<br>
don't feel /super/ great about it, but yeah, if we're going to push<br>
the Programmer's Manual encouragement further in terms of<br>
reducing/removing use of std::vector, then maybe llvm::Vector isn't<br>
the worst way to do that.<br>
<br>
(it might be that this would benefit from being two separate<br>
discussions, though - one on how to provide smart defaults for<br>
SmallVector<T> and one on if/how to push std::vector deprecation in<br>
favor of SmallVector<T, 0> (semantically speaking - no matter how it's<br>
spelled) further along)<br></blockquote><div><br></div><div>For historical purpose: the review was actually originally only adding a default for SmallVector and nothing else :)</div><div>We only converged to the current proposal after a few rounds of reviews trying to tradeoff amongst folks in the review.</div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Cheers,<br>
><br>
> --<br>
> Mehdi<br>
><br>
><br>
>><br>
>><br>
>> On Fri, Nov 13, 2020 at 2:06 PM Sean Silva via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> ><br>
>> > We've pretty happy now with a patch that adds two wrappers around SmallVector that make it 1) more convenient to use and 2) will tend to mitigate misuse of SmallVector. We think it's ready for wider discussion: <a href="https://reviews.llvm.org/D90884" rel="noreferrer" target="_blank">https://reviews.llvm.org/D90884</a><br>
>> ><br>
>> > SVec<T> is a convenience alias for SmallVector<T, N> with N chosen automatically to keep its size under 64 Bytes (that heuristic is easy to change though). The recommendation in the patch is to use this "on the stack, where a "small" number of elements are expected".<br>
>> ><br>
>> > Vec<T> is a convenience alias for SmallVector<T, 0>. It lets us get the (little-known?) benefits of SmallVector even when it has no inline elements (see <a href="https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h" rel="noreferrer" target="_blank">https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h</a>). The recommendation in the patch is to use this when the SmallVector is on the heap.<br>
>> ><br>
>> > A lot of this is boiled out from the discussion in <a href="https://groups.google.com/g/llvm-dev/c/q1OyHZy8KVc/m/1l_AasOLBAAJ?pli=1" rel="noreferrer" target="_blank">https://groups.google.com/g/llvm-dev/c/q1OyHZy8KVc/m/1l_AasOLBAAJ?pli=1</a><br>
>> ><br>
>> > The goals here are twofold:<br>
>> ><br>
>> > 1. convenience: not having to read/write "N", or do an extra edit/recompile cycle if you forgot it<br>
>> ><br>
>> > 2. avoiding pathological cases: The choice of N is usually semi-arbitrary in our experience, and if one isn't careful, can result in sizeof(SmallVector) becoming huge, especially in the case of nested SmallVectors. This patch avoids pathological cases in two ways:<br>
>> > A. SVec<T>'s heuristic keeps sizeof(SVec<T>) bounded, which prevents pathological size amplifications like in `SmallVector<struct {SmallVector<T, 4> a, b; }, 4>`, where the small sizes effectively multiply together. Of course, users can always write SmallVector<T, N> explicitly to bypass this, but the need for that seems rare.<br>
>> > B. SmallVector<T, 0> feels "weird to write" for most folks, even though it is frequently the right choice. Vec<T> mitigates that by "looking natural".<br>
>> ><br>
>> > I'm surfacing this as an RFC to get feedback on a couple higher-level points:<br>
>> > - does everybody agree that SVec<T> and Vec<T> are useful to have?<br>
>> > - get wider consensus around suggesting these as "defaults" (see my updates to ProgrammersManual.rst in the patch)<br>
>> > - how much we want to bulk migrate code vs let it organically grow. Replacing SmallVector<T, 0> with Vec<T> should be completely mechanical. Replacing SmallVector<T, N> for general N would be a lot more work.<br>
>> > - of course: naming. SVector/Vector were floated in the patch as well and seem ok. SmallVec was rejected as it was a prefix of SmallVector (messes up autocomplete).<br>
>> ><br>
>> > Looking forward to a world with fewer guessed SmallVector sizes,<br>
>> ><br>
>> > -- Sean Silva<br>
>> > _______________________________________________<br>
>> > LLVM Developers mailing list<br>
>> > <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> > <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>