Bug 75050 – REGRESSION(r103349): 7.5% peak memory use increase on Windows

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Chang]

http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=115372&graph=vm_peak_r

Looks like the regression range is http://trac.webkit.org/log/trunk/Source?rev=103365&stop_rev=103354&verbose=on .

None of the changes seems to obviously be the cause.

I'm OOO right now, but thought I should file a bug so someone can take a look.

[Ryosuke Niwa]

Suspicious revisions:

Merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac
http://trac.webkit.org/changeset/103354

Tightened up Vector<T>::append
http://trac.webkit.org/changeset/103356

[Coverity] Fix leak in V8HTMLDocument
http://trac.webkit.org/changeset/103358

Change adoptPtr(new ...) to ...::create in Page.cpp
http://trac.webkit.org/changeset/103365

[Ryosuke Niwa]

Looking at Chromium changes, WebKit roll r115310 (Chromium revision) definitely regressed the memory usage on Windows bot:
http://build.chromium.org/f/chromium/perf/vista-release-dual-core/moz/report.html?history=150&rev=-1&graph=vm_peak_r

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=115306:115310

[Ryosuke Niwa]

All regressions are P1.

[Ryosuke Niwa]

Apparently http://trac.webkit.org/changeset/103354 is specific to Mac so we can cross that out.

It seems very unlikely that either http://trac.webkit.org/changeset/103358 or http://trac.webkit.org/changeset/103365 can cause Windows-specific memory regression.

Given that I think http://trac.webkit.org/changeset/103356/trunk/Source/JavaScriptCore/wtf/Vector.h is the culprit. I don't see why it can cause memory use increase unless the compiler is generating bad code but then I wouldn't be surprised if that were the case since the comment being removed says it used to cause a compilation error.

[Ilya Tikhonovsky]

I confirm that the problem is in webkit.
I'm bisecting it. Looks like it is somewhere between 103344 and 103355

The suspicious revision is 103353

sizeof(RenderStyle) is 64 instead of 56 on Windows (x86)
 https://bugs.webkit.org/show_bug.cgi?id=74876
Reviewed by Ryosuke Niwa.
Move bit fields into a new class and use unsigned for all types so we
align at 4 byte bounds. Also move the initializers into the header
file (has the side benefit of not needing to duplicate the initializers
in 3 places).
Enable the compile assert on Windows.

[Ilya Tikhonovsky]

(In reply to comment #5)
> I confirm that the problem is in webkit.
> I'm bisecting it. Looks like it is somewhere between 103344 and 103355
> 
> The suspicious revision is 103353
> 
> sizeof(RenderStyle) is 64 instead of 56 on Windows (x86)
>  https://bugs.webkit.org/show_bug.cgi?id=74876
> Reviewed by Ryosuke Niwa.
> Move bit fields into a new class and use unsigned for all types so we
> align at 4 byte bounds. Also move the initializers into the header
> file (has the side benefit of not needing to duplicate the initializers
> in 3 places).
> Enable the compile assert on Windows.

Actually it is 103349. 
enable USE_SKIA_TEXT by default, replacing GDI for all text drawing

Revert it?

[Mike Reed]

I expect this is related to the large font-cache setting we have (in skia/skia.gyp). This gives us a big perf win on page-cyclers (faster than GDI) but will take more ram (which can be purged).

[Mike Reed]

The max cache size can be reduce (either in the gyp, or via a runtime API), or we can live with a larger peak, since that is the tradeoff for faster performance. I can go either way.

Here are more graphs, showing the simultaneous performance improvement and peak-memory-consumption regression as of Chromium r115310 (Webkit DEPS roll):

http://build.chromium.org/f/chromium/perf/vista-release-dual-core/intl1/report.html?history=150&rev=115500&graph=times
http://build.chromium.org/f/chromium/perf/vista-release-dual-core/intl1/report.html?history=150&rev=115500&graph=vm_peak_r

Are there any bots that went red as a result of this peak-memory-consumption regression?  I don't see any on the Chromium side...

[Mike Reed]

We will locally reproduce these page cyclers, and play with the font-cache size to mitigate the ram usage -vs- performance, but for the moment I recommend the code stays as is.
- faster text drawing needs ram unfortunately
- we will continue to improve our balance-point for ram-vs-speed as we gather in-the-field numbers of real font usage (not just for these cyclers)
- we will also implement font-cache-shrinking for tabs that go into the background

[Tony Chang]

Since this is an intentional trade off, seems like we can close this bug if we file bugs for the two follow up action items mentioned in comment #11.

[Mike Reed]

http://code.google.com/p/chromium/issues/detail?id=108979
http://code.google.com/p/chromium/issues/detail?id=108980

Closing, to track the follow-on initiatives via the above bugs.