1295192 - micro-optimize layout code by removing null checks associated with placement operator new

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove CPP_THROW_NEW on layout struct operator new overloads that forward to nsPresShell::AllocateByObjectID

Structs in our style system use an arena-style allocation system,
managed by the presshell to which they belong.  All of the relevant
overloads that forward allocation requests to the presshell declare
themselves as CPP_THROW_NEW, which indicates that they do not throw
exceptions.  The C++ specification states that operator new overloads
that declare themselves to not throw exceptions require a null check on
their return value.  However, the relevant presshell allocation method,
AllocateByObjectID, is infallible and will never return a null pointer.

The callers of all of these methods are therefore doing useless null
checks.  Let's get rid of those useless checks by removing the
CPP_THROW_NEW annotations.  This change declares these methods will
return non-null pointers and throw exceptions in case of errors--but as
we don't use exceptions, and AllocateByObjectID will abort on OOM,
everything works out OK.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove CPP_THROW_NEW from style struct type-safe operator new overloads

operator new overloads that declare that they don't throw exceptions
require a null check on their return value, per the C++ spec.  We know
that Servo isn't going to call these functions with null pointers, so
remove the CPP_THROW_NEW annotation and save ourselves some work.

We could add MOZ_ASSERTs in the generated wrappers from ServoBindings.cpp if
you wanted, just to make sure Servo isn't handing us bad data.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - remove CPP_THROW_NEW from display list code

nsDisplayListBuilder::Allocate is infallible.  Therefore,
nsDisplayListItem::operator new, which calls Allocate to obtain memory,
does not need to declare itself as throwing.  And so on for functions
that call nsDisplayListBuilder::Allocate.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - use a non-null-checking placement new operator in layout code

The standard placement new function is declared to not throw, which
means that, per spec, a null check on its result is required.  There are
a number of places throughout layout/ where we know that we are passing
non-null pointers to placement new (and receiving them as a return
value), and we are therefore doing useless work performing these null
checks.

Therefore, we should be using an operator new overload that doesn't
require the null check.  MFBT has just such an overload, so use that.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8781174 [details] [diff] [review]
part 2 - remove CPP_THROW_NEW from style struct type-safe operator new overloads

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

rs=me

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8781173 [details] [diff] [review]
part 1 - remove CPP_THROW_NEW on layout struct operator new overloads that forward to nsPresShell::AllocateByObjectID

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

Maybe change the extended commit message to clarify that the callers' null-checks are compiler-generated, as you explained over IRC?

e.g. replace this:
> The callers of all of these methods are therefore doing useless null
> checks.
...with this:
> The callers of all of these methods are therefore doing useless
> (compiler-generated) null-checks.
...or somesuch.

r=me in any case

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8781175 [details] [diff] [review]
part 3 - remove CPP_THROW_NEW from display list code

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

r=me

Comment 8

[Daniel Holbert [:dholbert]]

(Adding dependency on bug 1294537, which is where the OperatorNewExtensions.h header [used in part 4] was added very recently.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8781176 [details] [diff] [review]
part 4 - use a non-null-checking placement new operator in layout code

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

r=me, though please add assertions where it's not clear/guaranteed from contextual code that the pointer must in fact be non-null, as noted below:

::: layout/base/nsDisplayList.cpp
@@ +1121,5 @@
>                                                      bool aIsAsyncScrollable)
>  {
>    void* p = Allocate(sizeof(DisplayItemScrollClip));
>    DisplayItemScrollClip* c =
> +    new (KnownNotNull, p) DisplayItemScrollClip(aParent, aScrollableFrame, aClip, aIsAsyncScrollable);

Please rewrap this line (maybe before "aClip") so that it doesn't end up as hugely-long.

::: layout/style/nsCSSRuleProcessor.cpp
@@ +260,5 @@
>  static void
>  RuleHash_InitEntry(PLDHashEntryHdr *hdr, const void *key)
>  {
>    RuleHashTableEntry* entry = static_cast<RuleHashTableEntry*>(hdr);
> +  new (KnownNotNull, entry) RuleHashTableEntry();

Please add an assertion here to verify that the pointer is, in fact, not-null.  (Asserting either about |hdr| or |entry|, as you prefer -- before or after the static_cast.)

(Because: it's not clear from context here that the pointer must in fact be null; it's just a parameter, and some caller could conceivably pass in null by accident.  So passing KnownNotNull seems unjustified, and might trigger undefined behavior for all I know, if we violated its promise-of-non-nullness. So, might as well strenghten/backup our "KnownNotNull" annotation with an assert to make the non-null assumption more explicit/verified.)

@@ +278,5 @@
>    NS_PRECONDITION(from != to, "This is not going to work!");
>    RuleHashTableEntry *oldEntry =
>      const_cast<RuleHashTableEntry*>(
>        static_cast<const RuleHashTableEntry*>(from));
> +  RuleHashTableEntry *newEntry = new (KnownNotNull, to) RuleHashTableEntry();

Here as well, please add a MOZ_ASSERT to verify that |from| and |to| are non-null.

I'm pretty sure this function would crash in a variety of ways if either of these args were null.  But we don't have any clear null-checks or assertions that prevent us from getting null pointers here.  So, worth asserting to be sure our assumptions hold up, to sanity-check our callers, and to give the KnownNotNull a little more justification.

@@ +296,5 @@
>  static void
>  RuleHash_TagTable_InitEntry(PLDHashEntryHdr *hdr, const void *key)
>  {
>    RuleHashTagTableEntry* entry = static_cast<RuleHashTagTableEntry*>(hdr);
> +  new (KnownNotNull, entry) RuleHashTagTableEntry();

As above, please add an assertion to null-check |hdr| or |entry|.

@@ +315,5 @@
>    NS_PRECONDITION(from != to, "This is not going to work!");
>    RuleHashTagTableEntry *oldEntry =
>      const_cast<RuleHashTagTableEntry*>(
>        static_cast<const RuleHashTagTableEntry*>(from));
> +  RuleHashTagTableEntry *newEntry = new (KnownNotNull, to) RuleHashTagTableEntry();

As above, please add an assertion to null-check |from| and |to|.

@@ +794,5 @@
>  static void
>  AtomSelector_InitEntry(PLDHashEntryHdr *hdr, const void *key)
>  {
>    AtomSelectorEntry *entry = static_cast<AtomSelectorEntry*>(hdr);
> +  new (KnownNotNull, entry) AtomSelectorEntry();

As above, please add an assertion to null-check |hdr| or |entry|.

@@ +805,5 @@
>  {
>    NS_PRECONDITION(from != to, "This is not going to work!");
>    AtomSelectorEntry *oldEntry =
>      const_cast<AtomSelectorEntry*>(static_cast<const AtomSelectorEntry*>(from));
> +  AtomSelectorEntry *newEntry = new (KnownNotNull, to) AtomSelectorEntry();

As above, please add an assertion to null-check |from| and |to|.

@@ +3480,5 @@
>  static void
>  InitWeightEntry(PLDHashEntryHdr *hdr, const void *key)
>  {
>    RuleByWeightEntry* entry = static_cast<RuleByWeightEntry*>(hdr);
> +  new (KnownNotNull, entry) RuleByWeightEntry();

As above, please add an assertion to null-check |hdr| or |entry|.

::: layout/style/nsHTMLStyleSheet.cpp
@@ +236,5 @@
>  LangRuleTable_InitEntry(PLDHashEntryHdr *hdr, const void *key)
>  {
>    const nsString *lang = static_cast<const nsString*>(key);
>  
> +  LangRuleTableEntry *entry = new (KnownNotNull, hdr) LangRuleTableEntry();

As above, please add an assertion to null-check |hdr|.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #9)
> ::: layout/style/nsCSSRuleProcessor.cpp
> @@ +260,5 @@
> >  static void
> >  RuleHash_InitEntry(PLDHashEntryHdr *hdr, const void *key)
> >  {
> >    RuleHashTableEntry* entry = static_cast<RuleHashTableEntry*>(hdr);
> > +  new (KnownNotNull, entry) RuleHashTableEntry();
> 
> Please add an assertion here to verify that the pointer is, in fact,
> not-null.  (Asserting either about |hdr| or |entry|, as you prefer -- before
> or after the static_cast.)
> 
> (Because: it's not clear from context here that the pointer must in fact be
> null; it's just a parameter, and some caller could conceivably pass in null
> by accident.  So passing KnownNotNull seems unjustified, and might trigger
> undefined behavior for all I know, if we violated its
> promise-of-non-nullness. So, might as well strenghten/backup our
> "KnownNotNull" annotation with an assert to make the non-null assumption
> more explicit/verified.)

Does it make any difference to your comment (and the comments elsewhere on this patch) that:

1) The caller of these functions, PLDHashTable code, isn't going to pass in non-null pointers; and
2) The KnownNotNull overload already does a MOZ_ASSERT on the pointer it receives, in effect centralizing the check in one place?

Comment 11

[Daniel Holbert [:dholbert]]

(1) doesn't sway me too much.  (2) does sway me, though, I suppose. So, feel free to disregard comment 9 aside from the first nit about rewrapping.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f143a01f78
part 1 - remove CPP_THROW_NEW on layout struct operator new overloads that forward to nsPresShell::AllocateByObjectID; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d12eaa8074
part 2 - remove CPP_THROW_NEW from style struct type-safe operator new overloads; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e683dce4197a
part 3 - remove CPP_THROW_NEW from display list code; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f10779fe019
part 4 - use a non-null-checked placement new operator in layout code; r=dholbert

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out because it might have caused 32-bit linux debug crashtest assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=33957663&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7d45d6ebfa

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e323796605a
part 1 - remove CPP_THROW_NEW on layout struct operator new overloads that forward to nsPresShell::AllocateByObjectID; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b66848f748
part 2 - remove CPP_THROW_NEW from style struct type-safe operator new overloads; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded99645e6df
part 3 - remove CPP_THROW_NEW from display list code; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebd9f34c08e
part 4 - use a non-null-checked placement new operator in layout code; r=dholbert

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e323796605a
https://hg.mozilla.org/mozilla-central/rev/03b66848f748
https://hg.mozilla.org/mozilla-central/rev/ded99645e6df
https://hg.mozilla.org/mozilla-central/rev/6ebd9f34c08e