Bug 69238 – REGRESSION(96070) 25% intl1 PLT regression from scrollbar invalidation

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

[Adam Barth]

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

[Adam Barth]

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

[Adam Barth]

This regression shows up on Linux too:
http://build.chromium.org/f/chromium/perf/linux-release/intl1/report.html?history=300&rev=103677

[Adam Barth]

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

[Darin Fisher (:fishd, Google)]

I'm pretty sure that the Chromium page cyclers run with GPU accelerated rendering disabled.  That means no accelerated compositor.

[Adam Barth]

I can create a build from around this time frame and bisect.

[Adam Barth]

I'm having trouble reproducing these results locally on 10.6.  It might be easier on Windows, where the change is more pronounced.

[Darin Fisher (:fishd, Google)]

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.

[Simon Fraser (smfr)]

Hmm, we could try not doing the invalidations if the document is being destroyed.

[Adam Barth]

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.

[Sam Weinig]

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

[Adam Barth]

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

[Darin Fisher (:fishd, Google)]

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)

[Darin Fisher (:fishd, Google)]

It looks like reverting only http://trac.webkit.org/changeset/96070/trunk/Source/WebCore/platform/ScrollView.cpp is sufficient to resolve the performance regression.

[Darin Fisher (:fishd, Google)]

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

[Adam Barth]

Removing the [Chromium] label because the fix is going to involve cross-platform code.

[Adam Barth]

+darin because he reviewed the patch.

[Adam Barth]

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

[Darin Adler]

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?

[Darin Adler]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Adler]

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

[Simon Fraser (smfr)]

I'll get a patch today to reduce the invalidations.

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

[Simon Fraser (smfr)]

Is there a way I can get the "intl" PLT suite? I can't replicate the performance regression in the normal suite.

[Simon Fraser (smfr)]

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

[Darin Adler]

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

[Simon Fraser (smfr)]

I have not been able to reproduce it in normal PLT.

[Adam Barth]

If you mean the moz dataset, this issue doesn't seem to effect the moz dataset, as far as I can tell.

[Simon Fraser (smfr)]

So I don't have a way to reproduce this. Does the "intl" set have a lot of iframes, or form controls?

[Darin Fisher (:fishd, Google)]

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

[Simon Fraser (smfr)]

I have reproduced the regression in WK2 on Mac.

[Simon Fraser (smfr)]

Created attachment 110380 [details]
Patch

[Adam Barth]

> I have reproduced the regression in WK2 on Mac.

Thanks Simon.

[Darin Adler]

Comment on attachment 110380 [details]
Patch

The old code invalidated only when hiding. The new code invalidates on both hiding and showing. Is that intentional?

[Darin Adler]

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

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/97079

[Adam Barth]

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.