Bug 102822 – 2.5% regression on page cycler moz

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ojan Vafai]

It's a bit hard to see the regression, but it's definitely there. The way to see it is to compare the difference between the reference trace (t_ref), which does not have the new patches, to the non-reference traces, which do have the new patches. 

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz-http/report.html?rev=168773&graph=times&history=50
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?rev=168773&graph=times&history=50

Regression range: http://trac.webkit.org/log/?verbose=on&rev=135087&stop_rev=135070

Hard to be sure which of those patches is most likely. I would guess one of the antti/kling patches. But it could also be smfr. Or maybe one of haraken's V8 patches, although those were supposed to just be cleanups with no other effects.

[Ojan Vafai]

FWIW, I considered the possibility that r135082 was just undoing performance gains from r129644, but these graphs didn't show any performance gains at r129644.

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?rev=158973&graph=times&history=50

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?rev=158973&graph=times&history=50

[Ojan Vafai]

Looks like the www.nytimes.com_Table load time went from 77ms to 87ms and that accounts for the entirety of the regression.

[Ojan Vafai]

Further evidence that it was a WebKit side patch is that the regression happened on another bot at a non-overlapping chromium revision, but an overlapping webkit revision: http://build.chromium.org/f/chromium/perf/linux-release/moz/report.html?rev=168773&graph=times&history=50

[Ojan Vafai]

Actually, this seems to show up on some other bots as well:

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/moz/report.html?rev=168773&graph=times&history=50

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/moz/report.html?rev=168773&graph=times&history=50

And this point at the regression being in the following range:
http://trac.webkit.org/log/?verbose=on&rev=135077&stop_rev=135068

[Ojan Vafai]

Sigh. I'm not sure I trust any of these regression ranges except the large one from the chromium main waterfall (http://trac.webkit.org/log/?verbose=on&rev=135094&stop_rev=135047). Something clearly went wrong on the other bots here where the data from some of the runs is repeated. I suppose we can safely look at the webkit revision before the repetition and after and use that as the range. I'll try piecing this together.

[Ojan Vafai]

Bug with the Chromium bots for anyone that wants to follow along: http://crbug.com/162722.

In the meantime, I think I can get a better (accurate) regression range for this bug.

[Ojan Vafai]

OK, I looked at manually at the bot output for the linux release bot and we get more reasonable data out of it. At r135081 there was no regression and at r135087 there was.

Regression range: http://trac.webkit.org/log/?rev=135087&stop_rev=135081&verbose=on

Here's my raw output out of R of the median times for each run.
test times135066 times135067 times135068 times135069 times135077 times135081 times135087 times135094
www.nytimes.com_Table        78.4        79.4        79.8        77.5        80.2        78.3        87.6        87.2

[Simon Fraser (smfr)]

Looks like http://trac.webkit.org/changeset/135082 then.

[Ojan Vafai]

*** Bug 102696 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

Created attachment 176147 [details]
test case

Took forever, but I think this testcase accurately represents the regression. There's a ~10% regression on this test between r134994 and r135540. Those were just the revisions it was easy for me to test. I still think the regression range is the one listed in comment 7.

[Ojan Vafai]

Since we only measure the time of a document.write of a bunch of tables, I think that rules out the V8 changes as well. So, I think that does in fact only leave http://trac.webkit.org/changeset/135082 as a possible culprit.

[Andreas Kling]

Passing to Antti for comment, yo.

[Antti Koivisto]

I think there is an unnecessary style invalidation from that patch...

[Antti Koivisto]

Created attachment 177106 [details]
patch

Based on the traces this should fix the page cycler test.

The attached reduction does show something that has regressed but I think the case is not a significant component of the page cycler number. Trace of that points to a layout related regression. Might be worth separate bug.

[Andreas Kling]

Comment on attachment 177106 [details]
patch

Makes sense, r=me

[Build Bot]

Comment on attachment 177106 [details]
patch

Attachment 177106 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15055984

New failing tests:
fast/events/xsl-onload.xhtml
fast/parser/external-entities-in-xslt.xml
fast/xsl/transform-to-html.xml
fast/xsl/xslt-doc-noenc.xml
fast/xsl/sort-locale.xml
fast/xsl/utf8-chunks.xml
fast/xsl/xslt-nested-stylesheets.xml
fast/xsl/exslt-node-set.xml
fast/xsl/xslt-doc-enc.xml
fast/xsl/xslt-enc16to16.xml
fast/parser/xslt-with-html.xml
http/tests/misc/location-test-xsl-style-sheet.xml
fast/events/overflow-scroll-fake-mouse-move.html
fast/xsl/import-after-comment.xml
fast/xsl/xslt-entity-enc.xml
fast/xsl/xslt-enc16.xml
fast/xsl/sort-unicode.xml
fast/xsl/dtd-in-source-document.xml
fast/xsl/subframe-location.html
fast/xsl/xslt-missing-namespace-in-xslt.xml
fast/xsl/xslt-extra-content-at-end.xml
fast/xsl/xslt-enc-cyr.xml
fast/xsl/mozilla-tests.xml
fast/loader/javascript-url-in-object.html
fast/xsl/xslt-import-depth.xml
fast/xsl/xslt-enc.xml
fast/xsl/xslt-entity.xml
fast/xsl/document-function.xml

[WebKit Review Bot]

Comment on attachment 177106 [details]
patch

Attachment 177106 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15121268

New failing tests:
fast/events/xsl-onload.xhtml
fast/forms/range/slider-delete-while-dragging-thumb.html
http/tests/security/contentSecurityPolicy/xsl-allowed.php
fast/parser/external-entities-in-xslt.xml
http/tests/security/xss-DENIED-xsl-external-entity.xml
fast/xsl/sort-locale.xml
fast/xsl/exslt-node-set.xml
fast/xsl/mozilla-tests.xml
http/tests/security/contentSecurityPolicy/xsl-unaffected-by-style-src-2.php
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
fast/parser/xslt-with-html.xml
http/tests/misc/location-test-xsl-style-sheet.xml
fast/loader/text-document-wrapping.html
fast/text/international/hindi-spacing.html
http/tests/security/cookies/first-party-cookie-allow-xslt.xml
http/tests/security/local-user-CSS-from-remote.html
http/tests/security/contentSecurityPolicy/xsl-img-blocked.php
fast/xsl/import-after-comment.xml
fast/forms/range/slider-mouse-events.html
fast/xsl/subframe-location.html
fast/xsl/sort-unicode.xml
fast/xsl/dtd-in-source-document.xml
http/tests/xmlviewer/dumpAsText/xsl-stylesheet.xml
http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml
http/tests/security/cookies/third-party-cookie-blocking-xslt.xml
fast/xsl/document-function.xml
fast/loader/javascript-url-in-object.html
http/tests/security/xss-DENIED-xsl-document-redirect.xml
http/tests/security/xss-DENIED-xsl-document.xml
fast/forms/range/slider-onchange-event.html

[Antti Koivisto]

Created attachment 177860 [details]
patch2

[Build Bot]

Comment on attachment 177860 [details]
patch2

Attachment 177860 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15144729

New failing tests:
inspector/timeline/timeline-script-tag-1.html

[WebKit Review Bot]

Comment on attachment 177860 [details]
patch2

Attachment 177860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15151782

New failing tests:
inspector/timeline/timeline-script-tag-1.html
userscripts/user-style-all-frames.html

[Eric Seidel (no email)]

Comment on attachment 177106 [details]
patch

Cleared Andreas Kling's review+ from obsolete attachment 177106 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Antti Koivisto]

Created attachment 208251 [details]
simpler safer patch

[Andreas Kling]

Comment on attachment 208251 [details]
simpler safer patch

Seems like a genuinely ok thing to do.

[Build Bot]

Comment on attachment 208251 [details]
simpler safer patch

Attachment 208251 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1399053

New failing tests:
inspector/timeline/timeline-script-tag-1.html

[Build Bot]

Created attachment 208253 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Antti Koivisto]

https://trac.webkit.org/r153788
 
with rebase of inspector/timeline/timeline-script-tag-1.html. It is a progression.

[Csaba Osztrogonác]

Comment on attachment 177860 [details]
patch2

Cleared Ojan Vafai's review+ from obsolete attachment 177860 [details] so that this bug does not appear in http://webkit.org/pending-commit.