Closed Bug 730581 Opened 8 years ago Closed 8 years ago

Lazy content blaster to asynchronize some of the unlinking

Categories

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

12 Branch
x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
Currently in the worst case we end up calling UnbindFromTree(deep=true) several times for a 
node during unlinking. We should be able to do that fewer times if we tear down
subtrees from bottom to top. Also, we should be able to do that all asynchronously.

Doing all unlinking async is hard, but this approach would let us asynchronize
hopefully significant portion of it.
Blocks: 698919
Comment on attachment 600680 [details] [diff] [review]
WIP

Looking good on try.

eventloop keeps sLazyContentBlaster alive, and it then keeps other
LazyContentBlast objects alive via mNext.

500 in 
nsAutoTArray<nsCOMPtr<nsIContent>, 500> mSubtreeRoots;
is somewhat random, but it is chosen so that LazyContentBlast objects
should fill a jemalloc bucket quite well.

BlastSubtree is very much what unlink does, expect that it calls
BlastSubtree(child) before unbinding the child. That way we end up unbinding
leaf nodes first.
Attachment #600680 - Flags: review?(jst)
Attachment #600680 - Flags: review?(continuation)
I wonder if it makes sense to do the NodeType() check in Append instead if in BlastSubtree
That doesn't work. BlastSubtree is called recursively. It ends up handling text nodes.
Comment on attachment 600680 [details] [diff] [review]
WIP

Bah, there are some xpcshell failures.
Attachment #600680 - Flags: review?(jst)
Attachment #600680 - Flags: review?(continuation)
Attached patch patch (obsolete) — Splinter Review
Keep layout module alive until we've processed all the objects.
Attachment #600680 - Attachment is obsolete: true
Attachment #600720 - Flags: review?(jst)
Attachment #600720 - Flags: review?(continuation)
Blocks: 730639
This is what a CC of http://dev.w3.org/html5/spec/Overview.html looks like without lazy content blaster:

cc: mRuntimes[*]->BeginCycleCollection() took 2ms
cc: SelectPurple() took 45ms
cc: MarkRoots() took 396ms
cc: ScanRoots() took 20ms
cc: CollectWhite::Root took 29ms
cc: CollectWhite::Unlink took 249ms
cc: CollectWhite::Unroot took 466ms
cc: CollectWhite() took 745ms
cc: ClearGraph() took 16ms
cc: total cycle collector time was 1232ms
cc: visited 250846 ref counted and 1356 GCed objects, freed 249630 ref counted and 284 GCed objects.

This is what it looks like with the LCB:

cc: PrepareForCollection() took 7ms
cc: mRuntimes[*]->BeginCycleCollection() took 2ms
cc: SelectPurple() took 7ms
cc: MarkRoots() took 467ms
cc: ScanRoots() took 24ms
cc: CollectWhite::Root took 29ms
cc: CollectWhite::Unlink took 117ms
cc: CollectWhite::Unroot took 26ms
cc: CollectWhite() took 173ms
cc: ClearGraph() took 16ms
cc: total cycle collector time was 706ms
cc: visited 253045 ref counted and 13200 GCed objects, freed 249432 ref counted and 10187 GCed objects.

The MarkRoots phase takes about the same amount of time (this patch shouldn't affect it, so any difference is likely due to differences in when the GC etc ran), but Unlink and especially Unroot are much faster.
That is exactly what I expected. Unlinking should be a bit faster, because there is no
need to call nsIContent::Unbind(), and Unroot is faster because nodes are actually deleted 
asynchronously after cycle collection.
Comment on attachment 600720 [details] [diff] [review]
patch

+class LazyContentBlast : public nsRunnable

How about we name this something like ContentUnbinder? :)

+  void BlastSubtree(nsIContent* aNode)

UnbindSubtree()?

+  void Append(nsIContent* aSubtreeRoot)
+  {
+    if (mLast->mSubtreeRoots.Length() >= 500) {

Where did 500 come from? Should we at least put that in a constant somewhere, maybe 500 is more than we want on slow mobile systems? Or do you think it doesn't matter? If needed this could probably be changed to be time based rather than just based on a number of nodes, i.e. do x amount of unbinding, check how much time was spent, do more or dispatch a runnable if we've already spent more time than we want to be away from the event loop. Not sure if that's necessary, but wanted to surface that idea at least.

+  } else if (!tmp->GetParent() && tmp->mAttrsAndChildren.ChildCount()) {
+    if (!LazyContentBlast::sLazyContentBlaster) {
+      LazyContentBlast::sLazyContentBlaster = new LazyContentBlast();
+      nsCOMPtr<nsIRunnable> e = LazyContentBlast::sLazyContentBlaster;
+      NS_DispatchToMainThread(e);
+    }
+    LazyContentBlast::sLazyContentBlaster->Append(tmp);

Could we please hide all this in a static Append() method on LazyContentBlast (or, ContentUnbinder, as I suggested we name it), and make sLazyContentBlaster (sContentUnbinder) private?

r- just because I'd like to read over the new patch with the new names and static methods, but I think the logic here is good, and it seems to work very well, at least on a moderately fast desktop system!
Attachment #600720 - Flags: review?(jst) → review-
jst probably already mentioned this, but it would also be good to think about how we can track pause times in telemetry for the lazy unbinding.
Andrew, I meant to mention that, but forgot here. Thanks for the reminder!
Attachment #600720 - Flags: review?(continuation)
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #9)
> 
> How about we name this something like ContentUnbinder? :)
"Blast" is actually used in nsDocument for this kind of thing.
But I could call the class ContentUnbinder


> +  void Append(nsIContent* aSubtreeRoot)
> +  {
> +    if (mLast->mSubtreeRoots.Length() >= 500) {
> 
> Where did 500 come from? Should we at least put that in a constant
> somewhere, maybe 500 is more than we want on slow mobile systems? Or do you
> think it doesn't matter?
It needs to be large enough so that we don't create huge number of runnables.
But 500 is somewhat random.

> If needed this could probably be changed to be time
> based rather than just based on a number of nodes, i.e. do x amount of
> unbinding, check how much time was spent, do more or dispatch a runnable if
> we've already spent more time than we want to be away from the event loop.
> Not sure if that's necessary, but wanted to surface that idea at least.
Yeah, something like that could be nice, but we also don't want to
fill event loop with huge number of runnables.


> Could we please hide all this in a static Append() method on
> LazyContentBlast (or, ContentUnbinder, as I suggested we name it), and make
> sLazyContentBlaster (sContentUnbinder) private?
Yes! :)


> 
> r- just because I'd like to read over the new patch with the new names and
> static methods, but I think the logic here is good, and it seems to work
> very well, at least on a moderately fast desktop system!
It should work reasonable well on slower systems too. The patch does split
part of the stuff that unlinking does to happen asynchronously. Whether or not
the asynchronous part could be split even to smaller pieces is kind of a different question.
Need to evaluate different values. With 500 we probably create usually only one runnable, 
but there are case, like innerHTML +=, where we may create tons of disconnected
content trees.

(Bug 730639 helps a lot with innerHTML += case, but there is some mysterious leak I can't
reproduce locally.)
OS: Linux → Mac OS X
Attached patch patchSplinter Review
Attachment #600720 - Attachment is obsolete: true
Attachment #601832 - Flags: review?(jst)
Comment on attachment 601832 [details] [diff] [review]
patch

Looks good!
Attachment #601832 - Flags: review?(jst) → review+
Comment on attachment 601832 [details] [diff] [review]
patch

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

::: content/base/src/nsGenericElement.cpp
@@ +4404,5 @@
> +    if (childCount) {
> +      while (childCount-- > 0) {
> +        // Hold a strong ref to the node when we remove it, because we may be
> +        // the last reference to it.  We need to call TakeChildAt() and
> +        // update mFirstChild before calling UnbindFromTree, since this last

"since this last"?  Maybe missing a word or something?

@@ +4423,5 @@
> +  {
> +    nsAutoScriptBlocker scriptBlocker;
> +    PRUint32 len = mSubtreeRoots.Length();
> +    PRTime start;
> +    if (len) {

Can you combine the PR_Now(), UnbindSubtree loop, the clear, and the accumulate under a single len branch?
> > +        // Hold a strong ref to the node when we remove it, because we may be
> > +        // the last reference to it.  We need to call TakeChildAt() and
> > +        // update mFirstChild before calling UnbindFromTree, since this last
> 
> "since this last"?  Maybe missing a word or something?

What happened... I copy pasted the text on purpose from unlink. Apparently something is missing.

> Can you combine the PR_Now(), UnbindSubtree loop, the clear, and the
> accumulate under a single len branch?
I can.
The 'this last' refers to the previous sentence.
https://hg.mozilla.org/mozilla-central/rev/7672adec56b9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
No longer blocks: 698919
Blocks: 698919
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.