Bug 67581 - REGRESSION (r87351): toggling display of lots (thousands) of elements with display:none is very slow
Summary: REGRESSION (r87351): toggling display of lots (thousands) of elements with di...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-04 18:02 PDT by Mike Lawther
Modified: 2011-09-11 20:24 PDT (History)
6 users (show)

See Also:


Attachments
showhide.html (206.04 KB, text/html)
2011-09-04 18:04 PDT, Mike Lawther
no flags Details
Patch (5.04 KB, patch)
2011-09-09 16:43 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (7.29 KB, patch)
2011-09-10 13:39 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2011-09-04 18:02:55 PDT
As reported at http://crbug.com/94248 :

1.  Create a bunch (2000 table rows) of elements
2.  Hide them using CSS (display:none)
3.  Show them using CSS (display:none)

See attached file (showhide.html) for repro.

Safari Version 5.0.4 (6533.20.27) (MacOS 10.6) - takes approx 1 sec to toggle

WebKit Nightly r93043 (MacOS 10.6) - takes > 30 secs to toggle
Chrome 15.0.865.0 dev (MacOS 10.6) - takes > 30 secs to toggle
Comment 1 Mike Lawther 2011-09-04 18:04:06 PDT
Created attachment 106297 [details]
showhide.html
Comment 2 Alexey Proskuryakov 2011-09-05 14:02:59 PDT
<rdar://problem/10075663>
Comment 3 Dimitri Glazkov (Google) 2011-09-08 11:11:10 PDT
Morrita-san, Antti thinks this might have been caused by NodeRenderingContext/NodeRendererFactory refactoring.
Comment 4 Antti Koivisto 2011-09-08 11:39:45 PDT
Sample showing it spending time under WebCore::NodeRenderingContext::nextRenderer():

Call graph:
    2483 Thread_37033   DispatchQueue_1: com.apple.main-thread  (serial)
    + 2483 ???  (in Safari)  load address 0x109e36000 + 0xf24  [0x109e36f24]
    +   2483 SafariMain  (in Safari) + 197  [0x7fff955e4725]
    +     2483 NSApplicationMain  (in AppKit) + 867  [0x7fff90e7652a]
    +       2483 -[NSApplication run]  (in AppKit) + 548  [0x7fff90bf842b]
    +         2483 -[BrowserApplication sendEvent:]  (in Safari) + 822  [0x7fff9543247a]
    +           2483 -[NSApplication sendEvent:]  (in AppKit) + 5665  [0x7fff90c61f19]
    +             2483 -[BrowserWindow sendEvent:]  (in Safari) + 378  [0x7fff954897e8]
    +               2483 -[Window sendEvent:]  (in Safari) + 115  [0x7fff956820c5]
    +                 2483 -[NSWindow sendEvent:]  (in AppKit) + 6478  [0x7fff90cc9734]
    +                   2483 -[WebHTMLView mouseUp:]  (in WebKit) + 223  [0x10a24347f]  WebHTMLView.mm:3639
    +                     2483 WebCore::EventHandler::mouseUp(NSEvent*)  (in WebCore) + 206  [0x10acbc39e]  EventHandlerMac.mm:527
    +                       2483 WebCore::EventHandler::handleMouseReleaseEvent(WebCore::PlatformMouseEvent const&)  (in WebCore) + 1024  [0x10acb6b50]  EventHandler.cpp:1717
    +                         2483 WebCore::EventHandler::handleMouseReleaseEvent(WebCore::MouseEventWithHitTestResults const&)  (in WebCore) + 618  [0x10acb343a]  EventHandler.cpp:760
    +                           2483 WebCore::FrameSelection::notifyRendererOfSelectionChange(WebCore::EUserTriggered)  (in WebCore) + 39  [0x10ad156c7]  FrameSelection.cpp:1690
    +                             2483 WebCore::Document::updateStyleIfNeeded()  (in WebCore) + 67  [0x10aba5253]  Document.cpp:1621
    +                               2483 WebCore::Document::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 459  [0x10aba446b]  Document.cpp:1564
    +                                 2483 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 2378  [0x10aca705a]  Element.cpp:1161
    +                                   2483 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 2378  [0x10aca705a]  Element.cpp:1161
    +                                     2483 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 2378  [0x10aca705a]  Element.cpp:1161
    +                                       2483 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 2378  [0x10aca705a]  Element.cpp:1161
    +                                         2483 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)  (in WebCore) + 653  [0x10aca699d]  Node.h:761
    +                                           2473 WebCore::Element::attach()  (in WebCore) + 101  [0x10aca5d35]  Element.cpp:974
    +                                           ! 2464 WebCore::Node::attach()  (in WebCore) + 79  [0x10b277f7f]  Node.cpp:1464
    +                                           ! : 2435 WebCore::NodeRendererFactory::createRendererIfNeeded()  (in WebCore) + 122  [0x10b281b6a]  NodeRenderingContext.cpp:349
    +                                           ! : | 1703 WebCore::NodeRenderingContext::nextRenderer() const  (in WebCore) + 270  [0x10b28151e]  Node.h:437
    +                                           ! : | 343 WebCore::NodeRenderingContext::nextRenderer() const  (in WebCore) + 287  [0x10b28152f]  NodeRenderingContext.cpp:184
    +                                           ! : | + 343 WebCore::Node::isContentElement() const  (in WebCore) + 0,6,...  [0x10aa42530,0x10aa42536,...]  Node.h:215
    +                                           ! : | 333 WebCore::NodeRenderingContext::nextRenderer() const  (in WebCore) + 278,287,...  [0x10b281526,0x10b28152f,...]  NodeRenderingContext.cpp:184
    +                                           ! : | 56 WebCore::NodeRenderingContext::nextRenderer() const  (in WebCore) + 251,255  [0x10b28150b,0x10b28150f]  Node.h:151
    +                                           ! : 27 WebCore::NodeRendererFactory::createRendererIfNeeded()  (in WebCore) + 133  [0x10b281b75]  NodeRenderingContext.cpp:350
Comment 5 Simon Fraser (smfr) 2011-09-08 11:46:51 PDT
Before NodeRenderingContext this was probably just walking RenderObjects. Is it the code that was added for coalescing adjacent text nodes?
Comment 6 Dimitri Glazkov (Google) 2011-09-08 11:57:52 PDT
Ouch, We shouldn't need to be doing that  isContentElement check. The ShadowContentElement is meaningless in the non-shadow DOM tree!
Comment 7 Dimitri Glazkov (Google) 2011-09-08 14:08:17 PDT
I am working on this.
Comment 8 Dimitri Glazkov (Google) 2011-09-08 21:00:34 PDT
(In reply to comment #7)
> I am working on this.

The patch that regressed this made nextRenderer() call even if the newRenderer is 0. And that's slow. I'll have patch ready tomorrow morning.
Comment 9 Dimitri Glazkov (Google) 2011-09-09 16:43:46 PDT
Created attachment 106939 [details]
Patch
Comment 10 Dimitri Glazkov (Google) 2011-09-09 16:44:55 PDT
There's probably a perf test I should write for this. But let's fix the bug first.
Comment 11 Darin Adler 2011-09-09 17:43:26 PDT
Comment on attachment 106939 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Covered by existing tests.

Maybe the code is covered, but is the performance covered? Can we make a performance test to demonstrate what is fixed here? I think the performance test machinery Ojan create makes that kind of thing easy.
Comment 12 Dimitri Glazkov (Google) 2011-09-10 13:37:31 PDT
Comment on attachment 106939 [details]
Patch

will do.
Comment 13 Dimitri Glazkov (Google) 2011-09-10 13:39:04 PDT
Created attachment 106981 [details]
Patch for landing
Comment 14 WebKit Review Bot 2011-09-11 20:24:10 PDT
Comment on attachment 106981 [details]
Patch for landing

Clearing flags on attachment: 106981

Committed r94938: <http://trac.webkit.org/changeset/94938>
Comment 15 WebKit Review Bot 2011-09-11 20:24:16 PDT
All reviewed patches have been landed.  Closing bug.