The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 10

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

10 Branch
mozilla12
Points:
---

Firefox Tracking Flags

(firefox10+ fixed, firefox11+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 583627 [details] [diff] [review]
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.
https://tbpl.mozilla.org/?tree=Try&rev=4d405a408197
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
Created attachment 583642 [details] [diff] [review]
no child nodes check

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

XHR blackness check didn't seem to be too useful.
With the latter patch I don't get high cc times with
https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=574869
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
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.
Attachment #583627 - Attachment is obsolete: true
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).
Attachment #583642 - Flags: review?(continuation)
Attachment #583642 - Flags: feedback?(bent.mozilla)
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.
Attachment #583642 - Flags: feedback?(jonas)
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?
Attachment #583642 - Flags: review?(continuation) → review+
(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.
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.
...well among other reasons. XBL elements don't usually have script objects, so checking
blackness doesn't make much sense.
Yeah, it seems reasonable to me to skip them.
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?
Yeah, I started think that myself last night. We probably don't need to check.
We certainly do need to check parent though.
(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.
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.
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.
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
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.
NODE_MAY_BE_IN_BINDING_MNGR is definitely needed. The parent must not have a binding.
Using GetBindin() or such would be slow.
And again, I'd rather be safe for FF10 (if this could land there).
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).
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.
Ah that case. We'll you just wanted to not check whether node is in anon content ;)
Created attachment 583950 [details] [diff] [review]
v3
Attachment #583642 - Attachment is obsolete: true
Attachment #583642 - Flags: feedback?(jonas)
Attachment #583642 - Flags: feedback?(bent.mozilla)
Attachment #583950 - Flags: review?(jonas)
https://tbpl.mozilla.org/?tree=Try&rev=95ac0656a201
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.
Attachment #583950 - Flags: review?(jonas) → review+
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.
Created attachment 583960 [details] [diff] [review]
v4

Even simpler. if this passes on try, I'll push this.
https://tbpl.mozilla.org/?tree=Try&rev=459f4e49b1a6
https://hg.mozilla.org/mozilla-central/rev/0697daa43413
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → bugs
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
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.
Attachment #583960 - Flags: approval-mozilla-beta?
Attachment #583960 - Flags: approval-mozilla-aurora?

Comment 35

5 years ago
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.
Attachment #583960 - Flags: approval-mozilla-beta?
Attachment #583960 - Flags: approval-mozilla-beta+
Attachment #583960 - Flags: approval-mozilla-aurora?
Attachment #583960 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/979022d58276
https://hg.mozilla.org/releases/mozilla-beta/rev/52aad9bbe2ff
status-firefox10: --- → fixed
status-firefox11: --- → fixed
Is there something QA can verify here?
Whiteboard: [qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #37)
> Is there something QA can verify here?

bump
No.
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.