Bug 98336 - Make RenderObject destruction during detach a top-down operation
Summary: Make RenderObject destruction during detach a top-down operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 92697
  Show dependency treegraph
 
Reported: 2012-10-03 18:06 PDT by Julien Chaffraix
Modified: 2012-10-16 18:26 PDT (History)
8 users (show)

See Also:


Attachments
Proposed change: change detach() to do a top-down destruction. (6.08 KB, patch)
2012-10-03 18:24 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread. (6.56 KB, patch)
2012-10-16 14:10 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-10-03 18:06:29 PDT
Currently, detach destroys the render tree in a bottom-up approach (see ContainerNode::detach).

While this is correct and will detach / destroy all renderers in the DOM subtree, it causes some extra-work to be done:
* RenderBlock tries to merge anonymous renderer when we remove a child (ie for each renderer in the subtree).
* RenderObject tries to clean any parent anonymous wrappers.
* We partially clean / destroy a tree level as the DOM doesn't know about the anonymous renderers. The leftover anonymous children will be cleaned up when their parent is destroyed.

Most of these operations only apply to the root of the subtree-to-detach, not on any renderer inside the subtree. The main issue is that detach is a DOM-driven operation and it doesn't know enough of the render tree structure to make some good decisions.

Also important is that the current logic in RenderObject already does top-down cleaning but the DOM logic overrides this behavior.
Comment 1 Julien Chaffraix 2012-10-03 18:24:03 PDT
Created attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.
Comment 2 Ojan Vafai 2012-10-03 19:05:13 PDT
Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

Do you actually see any performance benefit?
Comment 3 Julien Chaffraix 2012-10-03 20:32:26 PDT
(In reply to comment #2)
> (From update of attachment 167015 [details])
> Do you actually see any performance benefit?

I don't expect - nor have I seen - any performance improvement from this change. However it is a preparation for the real change(s) as the top-down approach is more tailored to them.
Comment 4 Eric Seidel (no email) 2012-10-15 17:23:05 PDT
Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

LGTM.
Comment 5 Darin Adler 2012-10-15 20:35:06 PDT
Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

View in context: https://bugs.webkit.org/attachment.cgi?id=167015&action=review

> Source/WebCore/dom/Node.cpp:1322
> +    ASSERT(!renderer());
> +#ifndef NDEBUG
> +    for (Node* n = this; n; n = n->traverseNextNode(this))
> +        ASSERT(!renderer());
> +#endif

The inner assertion should to say ASSERT(!n->renderer()), otherwise this just asserts the same thing over and over again.

The outer assertion would look nicer inside the #ifndef. But also, it’s redundant with the first assertion in the loop, although I suppose it might be more OK to repeat the assertion just to distinguish that case if it does fail.

We need to be careful about the exponential complexity here. This is going to make detach super-slow in debug builds, maybe unbearably so.
Comment 6 Julien Chaffraix 2012-10-16 09:40:39 PDT
Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

View in context: https://bugs.webkit.org/attachment.cgi?id=167015&action=review

>> Source/WebCore/dom/Node.cpp:1322
>> +#endif
> 
> The inner assertion should to say ASSERT(!n->renderer()), otherwise this just asserts the same thing over and over again.
> 
> The outer assertion would look nicer inside the #ifndef. But also, it’s redundant with the first assertion in the loop, although I suppose it might be more OK to repeat the assertion just to distinguish that case if it does fail.
> 
> We need to be careful about the exponential complexity here. This is going to make detach super-slow in debug builds, maybe unbearably so.

Good catch about the exponential complexity, it should be possible to avoid it and only walk the tree an extra time. Let me do more testing and come up with a better solution.
Comment 7 Julien Chaffraix 2012-10-16 14:10:54 PDT
Created attachment 169024 [details]
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread.
Comment 8 WebKit Review Bot 2012-10-16 18:26:42 PDT
Comment on attachment 169024 [details]
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread.

Clearing flags on attachment: 169024

Committed r131539: <http://trac.webkit.org/changeset/131539>
Comment 9 WebKit Review Bot 2012-10-16 18:26:46 PDT
All reviewed patches have been landed.  Closing bug.