Summary: | Make RenderObject destruction during detach a top-down operation | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | Layout and Rendering | Assignee: | 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
Julien Chaffraix
2012-10-03 18:06:29 PDT
Created attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.
Comment on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.
Do you actually see any performance benefit?
(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 on attachment 167015 [details]
Proposed change: change detach() to do a top-down destruction.
LGTM.
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 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. Created attachment 169024 [details]
Updated patch for landing: Fixed the ASSERT, avoid the bad complexity and tweaked it to account for flow-thread.
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> All reviewed patches have been landed. Closing bug. |