Bug 67581 – REGRESSION (r87351): toggling display of lots (thousands) of elements with display:none is very slow

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Mike Lawther]

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

[Mike Lawther]

Created attachment 106297 [details]
showhide.html

[Alexey Proskuryakov]

<rdar://problem/10075663>

[Dimitri Glazkov (Google)]

Morrita-san, Antti thinks this might have been caused by NodeRenderingContext/NodeRendererFactory refactoring.

[Antti Koivisto]

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

[Simon Fraser (smfr)]

Before NodeRenderingContext this was probably just walking RenderObjects. Is it the code that was added for coalescing adjacent text nodes?

[Dimitri Glazkov (Google)]

Ouch, We shouldn't need to be doing that  isContentElement check. The ShadowContentElement is meaningless in the non-shadow DOM tree!

[Dimitri Glazkov (Google)]

I am working on this.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

Created attachment 106939 [details]
Patch

[Dimitri Glazkov (Google)]

There's probably a perf test I should write for this. But let's fix the bug first.

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 106939 [details]
Patch

will do.

[Dimitri Glazkov (Google)]

Created attachment 106981 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 106981 [details]
Patch for landing

Clearing flags on attachment: 106981

Committed r94938: <http://trac.webkit.org/changeset/94938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.