730587 - Stash pointer to the subtree parent on nodes in disconnected subtrees

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Being able to always quickly get the subtree parent will make some CC optimizations more performant.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 601012 [details] [diff] [review]
Patch


>-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
>+  nsIFrame* GetPrimaryFrame() const {
{ should be in the next line.

>+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
ditto

>+  void ClearSubtreeRootPointer() {
And here.

> nsINode::~nsINode()
> {
>   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
>+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> }
Use stronger MOZ_ASSERT here?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 601012 [details] [diff] [review]
> Patch
> 
> 
> >-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
> >+  nsIFrame* GetPrimaryFrame() const {
> { should be in the next line.

Done.

> >+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
> ditto

Done.

> >+  void ClearSubtreeRootPointer() {
> And here.

Done.

> > nsINode::~nsINode()
> > {
> >   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
> >+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> > }
> Use stronger MOZ_ASSERT here?

It seems odd to have one fatal assert when we use NS_ASSERTION all over the DOM code.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Well, we haven't had MOZ_ASSERT for a long time.
In this case I really want debug builds to crash asap if something goes wrong.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 601033 [details] [diff] [review]
Patch

>+  nsIFrame* GetPrimaryFrame() const
>+  {
>+    return IsInDoc() ? mPrimaryFrame : nsnull;
I wonder if this shows up in the profiles. Probably not.


>+  union {
I think { should be in the next line.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 601033 [details] [diff] [review]
Patch

Looks good.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/5280e98d2d77

Comment 9

[Daniel Holbert [:dholbert]]

Pushed a followup to make OwnerDocAsNode non-inline, with rs=khuey:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bacfc9013c

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

We should consider backing this out of Aurora 14 due to bug 736695.

Comment 12

[Alex Keybl [:akeybl]]

Although backing this out will probably end up being the solution, we'll instead continue to track bug 736695 since it's the actual regression.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out of 14 to fix bug 736695.

https://hg.mozilla.org/releases/mozilla-aurora/rev/abe9bb783f88

Comment 14

[Boris Zbarsky [:bzbarsky]]

We need to back this out of Aurora 15 as well....

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

We have this backout tracked already in bug 736695 so not tracking here.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44103bdf65b1

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And once more, with feeling: https://hg.mozilla.org/releases/mozilla-aurora/rev/0f805ac53831