Bug 112125 – REGRESSION(r145305) Performance: 1.3% mac-release-10.6-webkit-latest/intl2/times/t change after rev 145300

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nils Barth]

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

[Nils Barth]

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

[Robert Hogan]

How do I access the source of the test that observed the regression?

[Robert Hogan]

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?

[Nils Barth]

Added Julien to CC list, in response to discussion at Chromium Issue 173154:
https://code.google.com/p/chromium/issues/detail?id=173154

[Nils Barth]

Opened bug at Chromium:
Issue 189102: Performance regression 1.3% in table layout baseline fix
http://crbug.com/189102

[Nils Barth]

(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

[Nils Barth]

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

[Nils Barth]

(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

[Robert Hogan]

Created attachment 192880 [details]
Patch

[Robert Hogan]

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.

[WebKit Review Bot]

Comment on attachment 192880 [details]
Patch

Clearing flags on attachment: 192880

Committed r145822: <http://trac.webkit.org/changeset/145822>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Fingers crossed, that appears to have done the trick:

http://chromium-perf.appspot.com/?tab=mac-release-10.6-webkit-latest&graph=times&trace=t&rev=188265&history=150&master=ChromiumWebkit&testSuite=page_cycler_intl2&details=true