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

RESOLVED FIXED in Firefox 10

Status

()

Core
DOM
RESOLVED FIXED
6 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)
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4d405a408197
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

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

Comment 6

6 years ago
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
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #583627 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
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)
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
...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?
(Assignee)

Comment 16

6 years ago
Yeah, I started think that myself last night. We probably don't need to check.
(Assignee)

Comment 17

6 years ago
We certainly do need to check parent though.
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
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.
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Comment 23

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

Comment 24

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

Comment 25

6 years ago
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.
(Assignee)

Comment 27

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

Comment 28

6 years ago
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)
(Assignee)

Comment 29

6 years ago
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.
(Assignee)

Comment 32

6 years ago
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
(Assignee)

Comment 33

6 years ago
https://hg.mozilla.org/mozilla-central/rev/0697daa43413
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Assignee: nobody → bugs
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
(Assignee)

Comment 34

6 years ago
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

6 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

6 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +
(Assignee)

Comment 36

6 years ago
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.