Bug 98336

Summary: Make RenderObject destruction during detach a top-down operation
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, esprehn, hyatt, inferno, ojan, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 92697    
Attachments:
Description Flags
Proposed change: change detach() to do a top-down destruction.
none
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread. none

Julien Chaffraix
Reported 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.
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Julien Chaffraix
Comment 1 2012-10-03 18:24:03 PDT
Created attachment 167015 [details] Proposed change: change detach() to do a top-down destruction.
Ojan Vafai
Comment 2 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?
Julien Chaffraix
Comment 3 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.
Eric Seidel (no email)
Comment 4 2012-10-15 17:23:05 PDT
Comment on attachment 167015 [details] Proposed change: change detach() to do a top-down destruction. LGTM.
Darin Adler
Comment 5 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.
Julien Chaffraix
Comment 6 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.
Julien Chaffraix
Comment 7 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.
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-10-16 18:26:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.