Closed Bug 1265892 Opened 3 years ago Closed 3 years ago

Change Vector to use Impl::new_ consistently.

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached patch vector-new.patchSplinter Review
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.
Attachment #8743008 - Flags: review?(jwalden+bmo)
Blocks: 1265898
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.
Attachment #8743664 - Flags: review?(sunfish)
Attachment #8743008 - Flags: review?(jwalden+bmo)
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.
Attachment #8743008 - Flags: review?(jwalden+bmo)
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.  :-(
Attachment #8743008 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/8926ca859714
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Attachment #8743664 - Flags: review?(sunfish)
Depends on: 1277377
You need to log in before you can comment on or make changes to this bug.