Bug 23662 - Browser hangs when link is clicked due to large number of calls to updateLayoutIgnorePendingStylesheets
Summary: Browser hangs when link is clicked due to large number of calls to updateLayo...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-01-31 03:27 PST by zextra
Modified: 2009-02-03 13:40 PST (History)
3 users (show)

See Also:


Attachments
Sample of main thread during hang (76.15 KB, text/plain)
2009-01-31 03:49 PST, Mark Rowe (bdash)
no flags Details
Compressed web archive of the page in its current state (7.69 MB, application/x-bzip2)
2009-01-31 03:59 PST, Mark Rowe (bdash)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description zextra 2009-01-31 03:27:37 PST
After page loads, when a link on the left is clicked (which execute some JS code), browser hangs for a long time. After it resumes operation, each subsequent click is very fast (or, so to say, normal-fast).

It should be noted that this behavior is specific to WebKit-powered browsers (Safari and Chrome, from what I know). Other browsers (all versions of IE, Firefox, Opera) do not have that issue.

Any thoughts?
Comment 1 Mark Rowe (bdash) 2009-01-31 03:46:05 PST
The following JS is run when one of the links is clicked:

function show_elems(filt)
{
        var all = $('.game-list .games > ul > li').hide();
       
        if (filt['soft'] != '') all = all.filter('.'+filt['soft']);
        if (filt['cat'] != '')  all = all.filter('.'+filt['cat']);
       
        all.show();
}

The call to jQuery's .hide() method appears to be responsible for much of the hang.  It is implemented as follows:

for ( var i = 0, l = this.length; i < l; i++ ){
	var old = jQuery.data(this[i], "olddisplay");
	if ( !old && old !== "none" )
		jQuery.data(this[i], "olddisplay", jQuery.css(this[i], "display"));
	this[i].style.display = "none";
}

jQuery.css is basically a wrapper around getComputedStyle.  This is looping over each element in the set, querying its computed style for the "display" property, then setting the display property to "none".  For this particular page there are approximately 1000 entries being iterated over.  The problem with this code is that modifying the style of a node causes the document layout info to need updating.  CSSComputedStyleDeclaration::getPropertyCSSValue always ensures that the document layout info is up to date before returning a value, which means that we're recalculating layout around 1000 times simply to hide a bunch of elements.
Comment 2 Mark Rowe (bdash) 2009-01-31 03:49:16 PST
Created attachment 27213 [details]
Sample of main thread during hang

The sample shows the majority of the time is spent calling Document::updateLayoutIgnorePendingStylesheets below CSSComputedStyleDeclaration::getPropertyCSSValue.
Comment 3 Mark Rowe (bdash) 2009-01-31 03:51:13 PST
This isn't code that I'm at all familiar with, but it seems to me that the "display" property isn't dependent on layout and therefore we don't need to be calling updateLayoutIgnorePendingStylesheets before returning its value.
Comment 4 Mark Rowe (bdash) 2009-01-31 03:54:02 PST
I suspect that a trivial change to jQuery could be made to avoid tickling this performance cliff.  If the "hide" method were change to use two separate loops it would avoid modifying the style info between each query of the "display" property and would therefore avoid the need to re-layout so many times.  Something like the following:

for ( var i = 0, l = this.length; i < l; i++ ){
	var old = jQuery.data(this[i], "olddisplay");
	if ( !old && old !== "none" )
		jQuery.data(this[i], "olddisplay", jQuery.css(this[i], "display"));
}
for ( var i = 0, l = this.length; i < l; i++ )
    this[i].style.display = "none";
Comment 5 Mark Rowe (bdash) 2009-01-31 03:55:58 PST
<rdar://problem/6546026>
Comment 6 Mark Rowe (bdash) 2009-01-31 03:59:01 PST
Created attachment 27214 [details]
Compressed web archive of the page in its current state

I'm attaching a web archive of the page in its current state in case the live site is modified.
Comment 7 Dave Hyatt 2009-01-31 16:51:29 PST
It should be possible to avoid doing either style updating or layout if we can detect that complex style rules aren't being used.  For example in the absence of crazy sibling selectors, if your element is not marked as "changed()" then your style information can be safely queried as long as the renderer itself is not being queried.

Comment 8 zextra 2009-02-03 13:40:06 PST
Original page that contains bug has been modified (uses fixed jquery, as suggested by bdash) and rellocated to another url, so please use archived version for further bug hunting.