69238 2011-10-02 22:25:36 -0700 REGRESSION(96070) 25% intl1 PLT regression from scrollbar invalidation 2011-10-10 14:13:02 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 69237 1 abarth simon.fraser darin fishd gregsimon jamesr leviw mitz sam simon.fraser oldest_to_newest 476774 0 abarth 2011-10-02 22:25:36 -0700 http://build.chromium.org/f/chromium/perf/xp-release-dual-core/intl1/report.html?history=100&rev=103031 http://trac.webkit.org/log/trunk?rev=96091&stop_rev=96066&verbose=on 476791 1 abarth 2011-10-02 22:52:43 -0700 Interesting revisions in this range: http://trac.webkit.org/changeset/96066 http://trac.webkit.org/changeset/96069 http://trac.webkit.org/changeset/96070 http://trac.webkit.org/changeset/96084 476793 2 abarth 2011-10-02 22:55:54 -0700 This regression shows up on Linux too: http://build.chromium.org/f/chromium/perf/linux-release/intl1/report.html?history=300&rev=103677 476794 3 abarth 2011-10-02 22:57:56 -0700 It's also on Mac, but it's harder to see: http://build.chromium.org/f/chromium/perf/mac-release-10.5/intl1/report.html?history=500&rev=103677 476796 4 fishd 2011-10-02 23:02:20 -0700 I'm pretty sure that the Chromium page cyclers run with GPU accelerated rendering disabled. That means no accelerated compositor. 476810 5 abarth 2011-10-02 23:23:54 -0700 I can create a build from around this time frame and bisect. 476841 6 abarth 2011-10-03 01:20:15 -0700 I'm having trouble reproducing these results locally on 10.6. It might be easier on Windows, where the change is more pronounced. 479620 7 fishd 2011-10-06 16:45:19 -0700 This particular change looks suspiciously like something that could impact performance: http://trac.webkit.org/changeset/96070/trunk/Source/WebCore/platform/ScrollView.cpp It is adding invalidations. 479624 8 simon.fraser 2011-10-06 16:49:29 -0700 Hmm, we could try not doing the invalidations if the document is being destroyed. 479656 9 abarth 2011-10-06 17:21:27 -0700 The first step is to validate that this is what's causing the issue. At the risk of making mitz sad, it might be worth temporarily removing the invalidations to see if that heals the bot. 479711 10 sam 2011-10-06 18:50:49 -0700 (In reply to comment #9) > The first step is to validate that this is what's causing the issue. At the risk of making mitz sad, it might be worth temporarily removing the invalidations to see if that heals the bot. Can this not be done locally on someones machine? 479720 11 abarth 2011-10-06 19:00:18 -0700 > Can this not be done locally on someones machine? I have not been able to reproduce the issue locally, but I think fishd was try to do that before he went home for the day. Apparently I can get ssh access to the perf bot if I ask nicely, but I haven't filled out the paperwork yet. 479848 12 fishd 2011-10-07 00:40:04 -0700 I have confirmed that this regression was indeed caused by http://trac.webkit.org/changeset/96070 I compared content_shell.exe performance with and without that patch using the intl1 page cycler. [with] 6365.25 (+/-279.66) 6631.25 (+/-179.05) 6513.50 (+/-184.78) [without] 5238.25 (+/-142.47) 5158.50 (+/-155.56) 5396.50 (+/-182.12) 479853 13 fishd 2011-10-07 00:54:26 -0700 It looks like reverting only http://trac.webkit.org/changeset/96070/trunk/Source/WebCore/platform/ScrollView.cpp is sufficient to resolve the performance regression. 479862 14 fishd 2011-10-07 01:04:40 -0700 (In reply to comment #13) > It looks like reverting only http://trac.webkit.org/changeset/96070/trunk/Source/WebCore/platform/ScrollView.cpp is sufficient to resolve the performance regression. Narrowing the scope further, this is not caused by the invalidateScrollCornerRect() call. It is somehow caused by the scrollbar invalidations. 480029 15 abarth 2011-10-07 08:48:34 -0700 Removing the [Chromium] label because the fix is going to involve cross-platform code. 480030 16 abarth 2011-10-07 08:49:00 -0700 +darin because he reviewed the patch. 480052 17 abarth 2011-10-07 09:04:22 -0700 @smfr: It looks like the regression exists on Snow Leopard too, but the magnitude is much smaller: http://build.chromium.org/f/chromium/perf/mac-release-10.6/intl1/report.html?history=80&rev=102938 Depending on which platform you're using, you might have more or less luck reproducing the issue. 480079 18 darin 2011-10-07 09:47:00 -0700 Dan, didn’t you make some code change related to this recently? I don’t mean a change that caused slowness, but a change that sped things up perhaps? Or avoided a flash? 480080 19 darin 2011-10-07 09:48:37 -0700 (In reply to comment #8) > Hmm, we could try not doing the invalidations if the document is being destroyed. Sounds like something we should try immediately. I also would not object to a rollout on principle, although it would be even better if we could move forward instead of backward. 480084 20 fishd 2011-10-07 09:54:48 -0700 (In reply to comment #19) > (In reply to comment #8) > > Hmm, we could try not doing the invalidations if the document is being destroyed. > > Sounds like something we should try immediately. > > I also would not object to a rollout on principle, although it would be even better if we could move forward instead of backward. Since Apple engineers also have a way to reproduce this regression locally, I'd really like to advocate for a rollout. Reason: I find that performance regressions can creep in and be hidden by existing performance regressions. Once you fix the original regression, you can end up not being back to where you thought you should be. Then you have a real mystery on your hands in trying to determine what other regression slipped in. The browser is a multi-threaded beast, and so one regression masking another is a real concern. 480086 21 darin 2011-10-07 09:59:13 -0700 (In reply to comment #20) > [...] I'd really like to advocate for a rollout. > > Reason: [...] [performance regressions hide others, etc.] Absolutely, the argument you are citing is one of the main WebKit project principles, even though we don’t always follow it religiously. It’s a great argument for a super-fast resolution. Obviously a rollout can be done faster than a fix. Simon, can you handle this yourself? 480100 22 simon.fraser 2011-10-07 10:18:15 -0700 I'll get a patch today to reduce the invalidations. 480104 23 mitz 2011-10-07 10:21:05 -0700 (In reply to comment #18) > Dan, didn’t you make some code change related to this recently? I don’t mean a change that caused slowness, but a change that sped things up perhaps? Or avoided a flash? I fixed the fact that invalidation could (and sometimes did) occur even when scrollbars were suppressed, but it doesn’t look like my fix eliminates any of the invalidation introduced by Simon in r96070. 480200 24 simon.fraser 2011-10-07 12:18:37 -0700 Is there a way I can get the "intl" PLT suite? I can't replicate the performance regression in the normal suite. 480275 25 simon.fraser 2011-10-07 13:32:01 -0700 One issue I notice is that SVGImages cause scrollbar invalidation when being torn down; they get to the host window via page->chrome() to do the invalidation. This seems wrong (but is unlikely to be the cause of perf issues). 480375 26 darin 2011-10-07 16:32:12 -0700 (In reply to comment #20) > Since Apple engineers also have a way to reproduce this regression locally Is this true? Do Apple engineers have a way to reproduce this regression? 480386 27 simon.fraser 2011-10-07 16:46:39 -0700 I have not been able to reproduce it in normal PLT. 480410 28 abarth 2011-10-07 17:19:57 -0700 If you mean the moz dataset, this issue doesn't seem to effect the moz dataset, as far as I can tell. 480412 29 simon.fraser 2011-10-07 17:29:40 -0700 So I don't have a way to reproduce this. Does the "intl" set have a lot of iframes, or form controls? 480479 30 fishd 2011-10-07 21:55:03 -0700 (In reply to comment #26) > (In reply to comment #20) > > Since Apple engineers also have a way to reproduce this regression locally > > Is this true? Do Apple engineers have a way to reproduce this regression? Whoops, I misread an earlier comment. (In reply to comment #29) > So I don't have a way to reproduce this. Does the "intl" set have a lot of iframes, or form controls? Let me see what can be done about that. 481024 31 simon.fraser 2011-10-10 11:20:16 -0700 I have reproduced the regression in WK2 on Mac. 481056 32 110380 simon.fraser 2011-10-10 12:15:25 -0700 Created attachment 110380 Patch 481067 33 abarth 2011-10-10 12:36:30 -0700 > I have reproduced the regression in WK2 on Mac. Thanks Simon. 481069 34 110380 darin 2011-10-10 12:40:56 -0700 Comment on attachment 110380 Patch The old code invalidated only when hiding. The new code invalidates on both hiding and showing. Is that intentional? 481071 35 darin 2011-10-10 12:45:23 -0700 (In reply to comment #34) > The old code invalidated only when hiding. The new code invalidates on both hiding and showing. Is that intentional? I figured it out. The new code does a null check so it only affects hiding. 481089 36 simon.fraser 2011-10-10 13:04:34 -0700 http://trac.webkit.org/changeset/97079 481144 37 abarth 2011-10-10 14:13:02 -0700 The perf graph looks good: http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=300&rev=-1 Thanks again. 110380 2011-10-10 12:15:25 -0700 2011-10-10 12:44:50 -0700 Patch bug-69238-20111010121524.patch text/plain 3610 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogOTY5NTIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBjNzRhMzg2ODg5M2IwZTA1 MDg5NmIzMjBjODhlY2NiOTU2NGNmZDUyLi5lYWU2NDhlMWRiZDQwZDJmMjUzOGRmYjc0ODVkY2Qz N2JjMzA4OWE4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjUgQEAKKzIwMTEtMTAtMTAgIFNpbW9u IEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTig5 NjA3MCkgMjUlIGludGwxIFBMVCByZWdyZXNzaW9uIGZyb20gc2Nyb2xsYmFyIGludmFsaWRhdGlv bgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjkyMzgK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggUExU IHJlZ3Jlc3Npb24gZnJvbSBhZGRpdGlvbmFsIHNjcm9sbGJhcnMgaW52YWxpZGF0aW9ucy4KKyAg ICAgICAgCisgICAgICAgIERvbid0IGludmFsaWRhdGUgdGhlIHNjcm9sbGJhciByZWN0IGZyb20g c2V0SGFzSG9yaXpvbnRhbFNjcm9sbGJhcigpCisgICAgICAgIGFuZCBzZXRIYXNWZXJ0aWNhbFNj cm9sbGJhcigpLCBiZWNhdXNlIHRoaXMgaXMgY2FsbGVkIGR1cmluZyBGcmFtZVZpZXcKKyAgICAg ICAgdGVhcmRvd24sIGF0IHdoaWNoIHBvaW50IHRoZSBpbnZhbGlkYXRpb24gaXMgcmVkdW5kYW50 LgorICAgICAgICAKKyAgICAgICAgSW5zdGVhZCwgaW52YWxpZGF0ZSBmcm9tIFNjcm9sbFZpZXc6 OnVwZGF0ZVNjcm9sbGJhcnMoKSwgd2hpY2ggaXMKKyAgICAgICAgY2FsbGVkIG9ubHkgd2hlbiBz Y3JvbGxiYXJzIGNoYW5nZSBiZWNhdXNlIG9mIHNpemUgb3IgbGF5b3V0CisgICAgICAgIGNoYW5n ZXMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlNjcm9sbFZpZXc6OnNldEhhc0hvcml6b250YWxTY3JvbGxiYXIpOgorICAgICAgICAoV2Vi Q29yZTo6U2Nyb2xsVmlldzo6c2V0SGFzVmVydGljYWxTY3JvbGxiYXIpOgorICAgICAgICAoV2Vi Q29yZTo6U2Nyb2xsVmlldzo6dXBkYXRlU2Nyb2xsYmFycyk6CisKIDIwMTEtMTAtMDcgIERhdmlk IEJhcnIgIDxkYXZpZGJhcnJAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEltcGxlbWVudCBjdXJy ZW50Q29sb3Igc3VwcG9ydCBmb3IgYm94LXNoYWRvdyBhbmQgdGV4dC1zaGFkb3cKZGlmZiAtLWdp dCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbFZpZXcuY3BwIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAKaW5kZXggNWY1YTcyNWZmYTMzYzAyY2JiNWI1MzQz NmEwNzJmODhmMzA0ZDdkOS4uNzhkMWVjZWQzZTU5YTI2MzZjMzE1NDFkMDQ2MDk2ZWYxYzEyYWVj YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAKQEAgLTk1LDcgKzk1LDYg QEAgdm9pZCBTY3JvbGxWaWV3OjpzZXRIYXNIb3Jpem9udGFsU2Nyb2xsYmFyKGJvb2wgaGFzQmFy KQogICAgICAgICBkaWRBZGRIb3Jpem9udGFsU2Nyb2xsYmFyKG1faG9yaXpvbnRhbFNjcm9sbGJh ci5nZXQoKSk7CiAgICAgICAgIG1faG9yaXpvbnRhbFNjcm9sbGJhci0+c3R5bGVDaGFuZ2VkKCk7 CiAgICAgfSBlbHNlIGlmICghaGFzQmFyICYmIG1faG9yaXpvbnRhbFNjcm9sbGJhcikgewotICAg ICAgICBtX2hvcml6b250YWxTY3JvbGxiYXItPmludmFsaWRhdGUoKTsKICAgICAgICAgd2lsbFJl bW92ZUhvcml6b250YWxTY3JvbGxiYXIobV9ob3Jpem9udGFsU2Nyb2xsYmFyLmdldCgpKTsKICAg ICAgICAgcmVtb3ZlQ2hpbGQobV9ob3Jpem9udGFsU2Nyb2xsYmFyLmdldCgpKTsKICAgICAgICAg bV9ob3Jpem9udGFsU2Nyb2xsYmFyID0gMDsKQEAgLTExNCw3ICsxMTMsNiBAQCB2b2lkIFNjcm9s bFZpZXc6OnNldEhhc1ZlcnRpY2FsU2Nyb2xsYmFyKGJvb2wgaGFzQmFyKQogICAgICAgICBkaWRB ZGRWZXJ0aWNhbFNjcm9sbGJhcihtX3ZlcnRpY2FsU2Nyb2xsYmFyLmdldCgpKTsKICAgICAgICAg bV92ZXJ0aWNhbFNjcm9sbGJhci0+c3R5bGVDaGFuZ2VkKCk7CiAgICAgfSBlbHNlIGlmICghaGFz QmFyICYmIG1fdmVydGljYWxTY3JvbGxiYXIpIHsKLSAgICAgICAgbV92ZXJ0aWNhbFNjcm9sbGJh ci0+aW52YWxpZGF0ZSgpOwogICAgICAgICB3aWxsUmVtb3ZlVmVydGljYWxTY3JvbGxiYXIobV92 ZXJ0aWNhbFNjcm9sbGJhci5nZXQoKSk7CiAgICAgICAgIHJlbW92ZUNoaWxkKG1fdmVydGljYWxT Y3JvbGxiYXIuZ2V0KCkpOwogICAgICAgICBtX3ZlcnRpY2FsU2Nyb2xsYmFyID0gMDsKQEAgLTQ5 Nyw2ICs0OTUsOCBAQCB2b2lkIFNjcm9sbFZpZXc6OnVwZGF0ZVNjcm9sbGJhcnMoY29uc3QgSW50 U2l6ZSYgZGVzaXJlZE9mZnNldCkKICAgICAgICAgaWYgKGhhc0hvcml6b250YWxTY3JvbGxiYXIg IT0gbmV3SGFzSG9yaXpvbnRhbFNjcm9sbGJhciAmJiAoaGFzSG9yaXpvbnRhbFNjcm9sbGJhciB8 fCAhYXZvaWRTY3JvbGxiYXJDcmVhdGlvbigpKSkgewogICAgICAgICAgICAgaWYgKG1fc2Nyb2xs T3JpZ2luLnkoKSAmJiAhbmV3SGFzSG9yaXpvbnRhbFNjcm9sbGJhcikKICAgICAgICAgICAgICAg ICBtX3Njcm9sbE9yaWdpbi5zZXRZKG1fc2Nyb2xsT3JpZ2luLnkoKSAtIG1faG9yaXpvbnRhbFNj cm9sbGJhci0+aGVpZ2h0KCkpOworICAgICAgICAgICAgaWYgKG1faG9yaXpvbnRhbFNjcm9sbGJh cikKKyAgICAgICAgICAgICAgICBtX2hvcml6b250YWxTY3JvbGxiYXItPmludmFsaWRhdGUoKTsK ICAgICAgICAgICAgIHNldEhhc0hvcml6b250YWxTY3JvbGxiYXIobmV3SGFzSG9yaXpvbnRhbFNj cm9sbGJhcik7CiAgICAgICAgICAgICBzZW5kQ29udGVudFJlc2l6ZWROb3RpZmljYXRpb24gPSB0 cnVlOwogICAgICAgICB9CkBAIC01MDQsNiArNTA0LDggQEAgdm9pZCBTY3JvbGxWaWV3Ojp1cGRh dGVTY3JvbGxiYXJzKGNvbnN0IEludFNpemUmIGRlc2lyZWRPZmZzZXQpCiAgICAgICAgIGlmICho YXNWZXJ0aWNhbFNjcm9sbGJhciAhPSBuZXdIYXNWZXJ0aWNhbFNjcm9sbGJhciAmJiAoaGFzVmVy dGljYWxTY3JvbGxiYXIgfHwgIWF2b2lkU2Nyb2xsYmFyQ3JlYXRpb24oKSkpIHsKICAgICAgICAg ICAgIGlmIChtX3Njcm9sbE9yaWdpbi54KCkgJiYgIW5ld0hhc1ZlcnRpY2FsU2Nyb2xsYmFyKQog ICAgICAgICAgICAgICAgIG1fc2Nyb2xsT3JpZ2luLnNldFgobV9zY3JvbGxPcmlnaW4ueCgpIC0g bV92ZXJ0aWNhbFNjcm9sbGJhci0+d2lkdGgoKSk7CisgICAgICAgICAgICBpZiAobV92ZXJ0aWNh bFNjcm9sbGJhcikKKyAgICAgICAgICAgICAgICBtX3ZlcnRpY2FsU2Nyb2xsYmFyLT5pbnZhbGlk YXRlKCk7CiAgICAgICAgICAgICBzZXRIYXNWZXJ0aWNhbFNjcm9sbGJhcihuZXdIYXNWZXJ0aWNh bFNjcm9sbGJhcik7CiAgICAgICAgICAgICBzZW5kQ29udGVudFJlc2l6ZWROb3RpZmljYXRpb24g PSB0cnVlOwogICAgICAgICB9Cg==