712743 - Investigate if we could reduce traversing by checking if node is black

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: WIP

Pushing this to try.
IsBlackNode(tmp) and IsBlackNode(parent) seem to catch some cases,
child list check not so often. Though, it really depends on testcases.

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=4d405a408197

Comment 3

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

I think the child nodes check is triggered so rarely and it can slow down
blackness checking significantly so it should be probably removed.

But XHR could have blackness check to prevent document traversing

Comment 4

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

Attachment: no child nodes check

https://tbpl.mozilla.org/?tree=Try&rev=88e2a4e7df7f

XHR blackness check didn't seem to be too useful.

Comment 5

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

With the latter patch I don't get high cc times with
https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=574869

Comment 6

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

But haven't got test results yet.

The patch applies cleanly to beta, so I pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=63659c69baa4

Comment 7

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

And to clarify, this is something I'm thinking for FF10 (and maybe FF11). 
Bug 705582 is way more effective, but also a lot more riskier.

Comment 8

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

Comment on attachment 583642 [details] [diff] [review]
no child nodes check

s/in black document/in a black document/

So, perhaps we should try this. And if we want to land something to
FF10, it must happen real soon (first to trunk ofc)

This patch should be reasonable safe (hopefully it even passes tests :) ).
I tried to leave out anon content, since those are anyway usually only in
in-cc-generation documents, and who owns what is trickier with
anon content.

The idea with the patch is that although some parts of a dom tree can be traversed,
once there is something black, we stop. That will hopefully cut down
graph size significantly at least in many cases. Pathological cases are still
possible (like they are with weak parent nodes).

Comment 9

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

To clarify, non-native anon nodes somewhere inside non-native anon content can be handled, since
those are kind of just normal subtrees, but the boundaries of anon and normal content are tricky, 
and the flags should prevent the problematic cases.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 583642 [details] [diff] [review]
no child nodes check

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

Looks good to me.  I don't really understand the problematicFlags stuff, so you should get somebody else to look at that.  It looked reasonable to me, and the other flags didn't seem like something to anything to worry about. r=me with that.

::: content/base/src/nsGenericElement.cpp
@@ +1246,5 @@
> +
> +      // If we're not in anonymous content and we have a black parent,
> +      // return early.
> +      nsIContent* parent = tmp->GetParent();
> +      if (parent && !IsXBL(parent) && !parent->HasFlag(problematicFlags) &&

Is there some reason you check IsXBL and HasFlag in a different order here than up above?

Comment 11

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

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Is there some reason you check IsXBL and HasFlag in a different order here
> than up above?
No. I'll fix that.

Comment 12

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

And btw, the IsXBL thing is there so that we don't check the odd <content> element which
binding keeps alive, but which isn't really in document.

Comment 13

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

...well among other reasons. XBL elements don't usually have script objects, so checking
blackness doesn't make much sense.

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, it seems reasonable to me to skip them.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do we need the problematic-flags check? Especially, why do we need to check the problematic flags if the node itself (rather than its document/parent) has a wrapper which is colored black?

Comment 16

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

Yeah, I started think that myself last night. We probably don't need to check.

Comment 17

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

We certainly do need to check parent though.

Comment 18

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

(In reply to Jonas Sicking (:sicking) from comment #15)
> Why do we need the problematic-flags check? Especially, why do we need to
> check the problematic flags if the node itself (rather than its
> document/parent) has a wrapper which is colored black?
Er, of course. If element is somewhere in anon content, document doesn't own it.

Comment 19

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

So, I prefer keeping as it is. It is just simpler and safer, and it shouldn't matter much, since
elements which are in anon content are almost always in in-cc-generation documents.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But if the element's wrapper is black, then someone clearly is holding onto the wrapper itself and the element isn't collectable. Removing that check seems to only remove code here, so I'm not fully getting the simpler argument.

Comment 21

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

It is not removing any code. Need to then check if node is "problematic" before checking whether
current document is black.

But  I can do that change if really wanted. I just don't see the point, since "problematic"
nodes really should be usually in a cc-generation document

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, well, it's not less code, it's basically the same amount though. So yes, I definitely think it's worth bailing any time the current node is black.

I'm also not sure why you need all of the flags that you are checking for here. All you're trying to do is to check for nodes that are anonymous, right? NODE_MAY_BE_IN_BINDING_MNGR and NODE_IS_INSERTION_PARENT definitely seems wrong for that. I would have thought that NODE_IS_IN_ANONYMOUS_SUBTREE is enough but I'd have to look it up.

Comment 23

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

NODE_MAY_BE_IN_BINDING_MNGR is definitely needed. The parent must not have a binding.
Using GetBindin() or such would be slow.

Comment 24

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

And again, I'd rather be safe for FF10 (if this could land there).

Comment 25

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

And... we don't have any good bits to check if something is in anon content (either native anon
or normal anon). GetBindingParent is easy way to check that, but it is possibly slow (virtual).

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do we care if the parent has a binding if the node itself isn't in anonymous content?

I guess keeping all the flags are ok with me for now, but please remove the flags-check on the node itself. We should always bail if the node's wrapper is black.

Comment 27

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

Ah that case. We'll you just wanted to not check whether node is in anon content ;)

Comment 28

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

Attachment: v3

Comment 29

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

https://tbpl.mozilla.org/?tree=Try&rev=95ac0656a201

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 583950 [details] [diff] [review]
v3

It's still more confusing than safe to me to be checking the flags on the parent.

But I can live with this I guess.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But please comment better on why you are checking those flags. I.e. something about that we want to make sure that the parent actually owns the child.

Comment 32

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

Attachment: v4

Even simpler. if this passes on try, I'll push this.
https://tbpl.mozilla.org/?tree=Try&rev=459f4e49b1a6

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/0697daa43413

Comment 34

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

Comment on attachment 583960 [details] [diff] [review]
v4

[Approval Request Comment]
Regression caused by (bug #): N/A 
Something in FF10 has regressed CC times, but I don't know what.

User impact if declined: N/A

Testing completed (on m-c, etc.):
Landed to m-c a week ago, and few devs have
tested tryserver builds (beta/FF10) and comments have been positive.
CC times have gone down a bit.

Risk to taking this patch (and alternatives if risky):
Risk is to find some bizarre bug which causes the black-ness check to
to be true too often. That could lead to leaks.

Comment 35

[LegNeato]

Comment on attachment 583960 [details] [diff] [review]
v4

[triage comment]
Approved for aurora and beta. If any issues are found we will back it out.

Comment 36

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/979022d58276
https://hg.mozilla.org/releases/mozilla-beta/rev/52aad9bbe2ff

Comment 37

[u279076]

Is there something QA can verify here?

Comment 38

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #37)
> Is there something QA can verify here?

bump

Comment 39

[Andrew McCreight [:mccr8]]

No.