Change Vector to use Impl::new_ consistently.

RESOLVED FIXED in Firefox 49

Status

()

Core
MFBT
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8743008 [details] [diff] [review]
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.
Attachment #8743008 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Blocks: 1265898
Created 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

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)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Updated

2 years ago
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+

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8926ca859714
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

2 years ago
Target Milestone: mozilla48 → mozilla49
(Assignee)

Updated

2 years ago
Attachment #8743664 - Flags: review?(sunfish)

Updated

2 years ago
Depends on: 1277377
You need to log in before you can comment on or make changes to this bug.