Proposal: permit absl::InlinedVector, remove base::StackVector

[Adam Rice]

absl::InlinedVector is an implementation of the std::vector API that stores up to a fixed number of elements inside the object itself, meaning that heap allocation can be avoided in common cases. Heap allocation is used if the built-in storage turns out not to be big enough.

WTF::Vector also has the same optimisation, but cannot be used outside of Blink.

base::StackVector is based on a similar idea, but has a slightly awkward API due to delegating to a std::vector internally. For example, you need to write v->push_back() rather than v.push_back(). It also takes a bit more memory because of the way it is implemented.

My proposal is to

1) Permit absl::InlinedVector, and comment that base::StackVector should not be used for new code.

2) Replace all users of base::StackVector with absl::InlinedVector (mostly trivial).

3) Remove base::StackVector and its supporting classes.

The benefit is less Chromium-specific code to maintain and a better implementation of this pattern.

I have a CL at https://chromium-review.googlesource.com/c/chromium/src/+/4659963 which simulates the effect of this change. There's a minor reduction in binary size and no performance impact.

The main use case for InlinedVector is small temporary vectors that are created during the lifetime of a function and then discarded.

InlinedVector should only be used in performance-critical functions. Other functions should continue to use std::vector + reserve() as it results in slightly smaller code. https://chromium-review.googlesource.com/c/chromium/src/+/4672187 demonstrates a 40 byte binary size increase from switching std::vector to absl::InlinedVector.

[K. Moon]

+1, doubt this is controversial.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC_ixdxajcbiKo9VQBqER%3Dk4%3DAtzjxxayKVoaApwv4hBj%2BETAQ%40mail.gmail.com.

[Elly Fong-Jones]

+1 :)

-- elly

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4H3HHEWfhpV%2BPRECv3CQyFaS0j%3DPs3uzd1VmZN2GNdPA%40mail.gmail.com.

[K. Moon]

https://chromium.googlesource.com/chromium/src/+/main/third_party/abseil-cpp/absl/container/inlined_vector.h uses ABSL_HARDENING_ASSERT(), so I don't think there should be any concerns around hardening. (If we find more hardening is needed, I'm sure the upstream Abseil team would be happy to accommodate us.)

[Anton Bikineev]

+1 to absl::InlinedVector in non-Blink code.

The only issue I can think of is the less informative diagnostics with ABSL_HARDENING_ASSERT() on non-official builds, which may result in more obscure ClusterFuzz reports (see similar issue for absl::optional). 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4yqo0g2CadkVAqoczTnopXostt7nLAtXczCA1Y4FHR%3DA%40mail.gmail.com.

[Joe Mason]

+1 from me, then!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4yqo0g2CadkVAqoczTnopXostt7nLAtXczCA1Y4FHR%3DA%40mail.gmail.com.

[Joe Mason]

Is absl::InlinedVector hardened against out-of-bounds accesses?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4H3HHEWfhpV%2BPRECv3CQyFaS0j%3DPs3uzd1VmZN2GNdPA%40mail.gmail.com.

[Adam Rice]

I couldn't find anything to add to c++-features.md, so I just removed InlinedVector from the banned containers list. How does this look: https://chromium-review.googlesource.com/c/chromium/src/+/4692682 ?

[Dana Jansens]

LGTM, thanks for being willing to do the work for this

--

[K. Moon]

Yes, many thanks!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRgzo3kw7TSwcBe3YS7ySyiWhqjW6aRN2%2BK8BJ6EoFnDA%40mail.gmail.com.