Bug 112125

Summary: REGRESSION(r145305) Performance: 1.3% mac-release-10.6-webkit-latest/intl2/times/t change after rev 145300
Product: WebKit Reporter: Nils Barth <nbarth>
Component: WebCore Misc.Assignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn+autocc, jchaffraix, ojan.autocc, robert, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Nils Barth 2013-03-12 03:10:53 PDT
There looks like there's a performance regression caused by r145305 fixing
Bug 108357 REGRESSION(r140907): Incorrect baseline for cells with media content during load

Concretely:
1.3% change in mac-release-10.6-webkit-latest/intl2/times/t after revision 187133 (WebKit revision 145300)
Graph:
http://chromium-perf.appspot.com/?details=true&tab=mac-release-10.6-webkit-latest&testSuite=intl2&rev=187433&graph=times&trace=t&history=150&master=ChromiumWebkit

(Pretty clear regression, narrow range, this revision changes layout, hence likely cause.)

I understand that this revision fixes a layout bug and is thus necessary in some form, but it looks like it has performance impact, perhaps due to additional computation and layout being necessary in some cases.

Could you take a look at it and see that any regressions are necessary and expected?
(Perhaps there's another way of doing it with less impact.)
Comment 1 Nils Barth 2013-03-12 03:12:39 PDT
Link added below:

(In reply to comment #0)
> There looks like there's a performance regression caused by r145305 fixing
> Bug 108357 REGRESSION(r140907): Incorrect baseline for cells with media content during load

r145305:
http://trac.webkit.org/changeset/145305
Bug 108357 REGRESSION(r140907): Incorrect baseline for cells with media content during load
Comment 2 Robert Hogan 2013-03-12 08:56:19 PDT
How do I access the source of the test that observed the regression?
Comment 3 Robert Hogan 2013-03-12 09:52:18 PDT
When I access the results page I can't seem to get anything other than a bunch of tabs. I don't get a graph or any other information. I'm using Chrome on Linux.

Any suggestions on what I need to do to view the regression?
Comment 4 Nils Barth 2013-03-12 18:52:28 PDT
Added Julien to CC list, in response to discussion at Chromium Issue 173154:
https://code.google.com/p/chromium/issues/detail?id=173154
Comment 5 Nils Barth 2013-03-12 19:41:50 PDT
Opened bug at Chromium:
Issue 189102: Performance regression 1.3% in table layout baseline fix
http://crbug.com/189102
Comment 6 Nils Barth 2013-03-12 23:08:24 PDT
(In reply to comment #2)
> How do I access the source of the test that observed the regression?

Command line looks to be:
/b/build/slave/Mac10_6_Perf/build/src/out/Release/performance_ui_tests --gtest_filter=PageCycler*.Intl2File:PageCycler*.*_Intl2File

So (on Mac 10.6) should be able to reproduce by running:
out/Release/performance_ui_tests --gtest_filter=PageCycler*.Intl2File:PageCycler*.*_Intl2File
Comment 7 Nils Barth 2013-03-12 23:10:54 PDT
(In reply to comment #3)
> When I access the results page I can't seem to get anything other than a bunch of tabs. I don't get a graph or any other information. I'm using Chrome on Linux.
> 
> Any suggestions on what I need to do to view the regression?

The graph takes a little while to load, and there's no feedback that it's loading.

I'm using Chrome on Linux as well, and this takes about 7 seconds to load:
http://chromium-perf.appspot.com/?tab=mac-release-10.6-webkit-latest&graph=times&trace=t&rev=187433&history=150&master=ChromiumWebkit&testSuite=page_cycler_intl2&details=true

Could you try viewing again?
The regression is about 80% of the way across; if you mouseover you'll see the Chromium and WebKit revision numbers.
Comment 8 Nils Barth 2013-03-12 23:12:49 PDT
(In reply to comment #6)
> (In reply to comment #2)
> > How do I access the source of the test that observed the regression?
> 
> Command line looks to be:
> /b/build/slave/Mac10_6_Perf/build/src/out/Release/performance_ui_tests --gtest_filter=PageCycler*.Intl2File:PageCycler*.*_Intl2File

BTW, this is determined by:
* clicking through to build bot waterfall (from graph page),
* search for test suite (on waterfall page)
* click on stdio
* eyeballing the stdio for a likely command line
Comment 9 Robert Hogan 2013-03-13 01:25:42 PDT
Created attachment 192880 [details]
Patch
Comment 10 Robert Hogan 2013-03-13 01:46:07 PDT
I'm pretty sure this will solve the regression and feel a bit dumb for letting it through in the first place. My attempt to detect cases where the delayed load of an image would push the baseline of the row down incorrectly unless it is allowed to move up into the headroom above did not consider the case where we're laying out the row for the first time so the row doesn't have a baseline yet. In that situation it was laying out the cell a second time unnecessarily.

The page cycler test suite is a set of popular websites as far as I can tell, so there's no specific use-case to test. If Julien is OK with the patch I guess we just watch the Performance bots after it lands.
Comment 11 WebKit Review Bot 2013-03-14 10:04:01 PDT
Comment on attachment 192880 [details]
Patch

Clearing flags on attachment: 192880

Committed r145822: <http://trac.webkit.org/changeset/145822>
Comment 12 WebKit Review Bot 2013-03-14 10:04:07 PDT
All reviewed patches have been landed.  Closing bug.