1265892 - Change Vector to use Impl::new_ consistently.

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: vector-new.patch

VectorImpl::new_ is an abstraction that allows Vector to avoid the C++ placement new nullptr check when constructing POD types. Currently it's used for Vector's append member function; this patch enables it emplaceBack and infallibleEmplaceBack as well, making them slightly faster under applicable conditions.

While here, it also updates Vector::new_ to use variadic templates to eliminate extra new_ overloads, and adds MOZ_NONNULL to help compilers that recognize that.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Change Vector to use Impl::new_ consistently, and extract a few VectorImpl methods into a base class shared between the POD/non-POD VectorImpls

Hmm.  Rather than double down on having these arguments as pointers, requiring compiler support to recognize the opportunity to eliminate the dereference, let's make them references.

And, why do we even have duplicates of some of these methods?  Some of them are 100% identical, so let's put them in a base class.  And others are identical once you recognize what POD implies.

This is a bigger patch, but it's also 61 insertions(+), 118 deletions(-), which seems even better.

Comment 2

[Dan Gohman [:sunfish]]

Comment on attachment 8743664 [details] [diff] [review]
Change Vector to use Impl::new_ consistently, and extract a few VectorImpl methods into a base class shared between the POD/non-POD VectorImpls

Review of attachment 8743664 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Vector.h
@@ +57,5 @@
> +   */
> +  template<typename... Args>
> +  static inline void new_(T& aDst, Args&&... aArgs)
> +  {
> +    // |aDst| is a reference so the compiler can eliminate a null-check.

Unfortunately, at least GCC 5.3.1 does not eliminate the null check in code written this way. As a practical matter, I'd prefer the original patch here.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8743008 [details] [diff] [review]
vector-new.patch

Review of attachment 8743008 [details] [diff] [review]:
-----------------------------------------------------------------

That's pretty egregious that gcc doesn't tag pointer values derived from the address of a reference as non-null.  :-(

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8926ca859714

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8926ca859714