Bug 98336 – Make RenderObject destruction during detach a top-down operation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

[Ojan Vafai]

Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

Do you actually see any performance benefit?

[Julien Chaffraix]

(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.

[Eric Seidel (no email)]

Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.

LGTM.

[Darin Adler]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 169024 [details]
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.