Last Comment Bug 725867 - Optimize anon content in BBP
: Optimize anon content in BBP
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2012-02-09 16:10 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-02-10 12:39 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (1.61 KB, patch)
2012-02-09 16:10 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: review+
continuation: feedback+
Details | Diff | Review
patch (2.20 KB, patch)
2012-02-10 11:42 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-09 16:10:56 PST
Created attachment 595898 [details] [diff] [review]
wip
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 06:43:46 PST
Comment on attachment 595898 [details] [diff] [review]
wip

This is still reasonable conservative, but helps with XUL based chrome UI
which has plenty of elements in anonymous content.

I'd like to find better way to ensure that anonymous elements in a document
are certainly alive, but haven't found that yet.
(document keeps non-anonymous elements alive)
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-02-10 09:24:55 PST
Comment on attachment 595898 [details] [diff] [review]
wip

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

::: content/base/src/nsGenericElement.cpp
@@ +4537,4 @@
>    nsIDocument* currentDoc = aNode->GetCurrentDoc();
>    if (currentDoc &&
> +      nsCCUncollectableMarker::InGeneration(currentDoc->GetMarkedCCGeneration()) &&
> +      // If aNode is not optimizable, but it is still an element

"but it is still" should probably be "but is"

@@ +4542,5 @@
> +      // we can act as if it was optimizable. When the primary frame dies, node
> +      // will end up to the purple buffer because of the refcount change.
> +      (!unoptimizable ||
> +       (currentDoc->GetShell() && aNode->IsElement() &&
> +        aNode->AsElement()->GetPrimaryFrame()))) {

Maybe turn these two lines into a separate function with a descriptive name, then move the comment up there?  I don't understand any of the unoptimizable stuff so I can't really comment on that.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-10 10:05:48 PST
Comment on attachment 595898 [details] [diff] [review]
wip

+      // If aNode is not optimizable, but it is still an element
+      // with a frame in a document which has currently active presshell,
+      // we can act as if it was optimizable. When the primary frame dies, node

"the node", or "aNode".

+      // will end up to the purple buffer because of the refcount change.
+      (!unoptimizable ||
+       (currentDoc->GetShell() && aNode->IsElement() &&
+        aNode->AsElement()->GetPrimaryFrame()))) {

What Andrew said. Maybe put the code after || in a static inlined helper named NodeHasFrame() or somesuch?

r=jst with that.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 11:42:19 PST
Created attachment 596124 [details] [diff] [review]
patch
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 12:39:27 PST
https://hg.mozilla.org/mozilla-central/rev/b3073c517d21

Note You need to log in before you can comment on or make changes to this bug.