868557 - Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

Clang and GCC probably MSVC are not able to unalias list because they don't realize that mEmptyChild and mChild can't alias mNextSibling and mPrevSibling. If we explicitly dereference list we get better code, saving 3 instructions on clang.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 745308 [details] [diff] [review]
Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)

>+  nsStyleContext **listPtr = aChild->mRuleNode->IsRoot() ? &mEmptyChild : &mChild;
>+  // Explicitly dereference listPtr so that compiler doesn't have to know that mNextSibling
>+  // etc. don't alias with what ever listPtr points at.
>+  nsStyleContext list = *listPtr;

This doesn't even look like it compiles; you shouldn't be able to assign an nsStyleContext* to an nsStyleContext, and you don't want to copy-construct an nsStyleContext ither.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Comment on attachment 745308 [details] [diff] [review]
> Explicitly unalias list in nsStyleContext::AddChild(nsStyleContext* aChild)
> 
> >+  nsStyleContext **listPtr = aChild->mRuleNode->IsRoot() ? &mEmptyChild : &mChild;
> >+  // Explicitly dereference listPtr so that compiler doesn't have to know that mNextSibling
> >+  // etc. don't alias with what ever listPtr points at.
> >+  nsStyleContext list = *listPtr;
> 
> This doesn't even look like it compiles; you shouldn't be able to assign an
> nsStyleContext* to an nsStyleContext, and you don't want to copy-construct
> an nsStyleContext ither.

Ooops, list should've been nsStyleContext *

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, r=dbaron with that, though I'm hoping the "saving 3 instructions" came from the correct patch rather than the one that I don't think should compile.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Try again

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f847ac098b3e