Closed Bug 1239151 Opened 4 years ago Closed 4 years ago

7% - 10% TPS OS X talos regression with Skia Content

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: [talos_regression][gfx-noted])

Attachments

(1 file)

With this test, the difference seems to come from the way Skia vs CG renders text. 

With CG, we prepare a glyph run and tell CG to render all the glyphs at once. e.g. "This" has 4 glyphs, so we'll call CTFontDrawGlyphs with 4 glyphs and be done.

Skia on the other hand, doesn't do this. Instead, it has a glyph cache and renders each glyph individually [1]. Skia then caches this glyph, so that if we use the same character and font size, it should be faster. However, this test iterates through 50 different webpages, 5 times each. The webpages are in different languages. The test goes through each of the 50 pages, and cycles through them instead of reloading the same page 5 times. e.g., if the list was google, yahoo, bing.com, the test goes through [google, yahoo, bing, google, yahoo, bing]. This thrashes the skia glyph cache, so when the test goes back to the same page at the next iteration of the cycle, Skia has to go back through each character and re-cache the glyph. This seems to cause the slowdown.

I think for the general use case where the user is browsing sites in generally the same language / fonts, the cache thrashing shouldn't be so bad, but I'll need to verify.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_mac.cpp?case=true&from=SkFontHost_mac.cpp#924
Whiteboard: [gfx-noted] → [talos_regression][gfx-noted]
Increase the skia font cache size to 10mb. This actually improves talos results[1] on this test. 10mb is enough where we don't have to flush the glyph cache at all during the tps test, fixing the regression. The default skia glyph cache size is 2mb. Chrome uses 20mb on desktop. This patch increases it to 10mb on non-android platforms. Chrome uses 1mb on android.


[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b33a31a16355&newProject=try&newRevision=b07745850776&framework=1&showOnlyImportant=0
Attachment #8732892 - Flags: review?(lsalzman)
Comment on attachment 8732892 [details] [diff] [review]
Increase skia font cache size to 10mb

It might be good to add a comment that we don't override the cache size on Android to conserve memory.
Attachment #8732892 - Flags: review?(lsalzman) → review+
https://hg.mozilla.org/mozilla-central/rev/99dcc91e0afd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Just an FYI for Eric and Nicholas if they see a 10mb spike in memory on OS X.
(In reply to Mason Chang [:mchang] from comment #7)
> Just an FYI for Eric and Nicholas if they see a 10mb spike in memory on OS X.

Is this a per process regression or just in the parent process? Either way a 10MiB is a bad thing, it gets worse when you have multiple content processes which is an active concern (we're chipping away in KiB at this point).

So, I really think we should back this out and reevaluate whether or not we should just eat this regression.
FWIW, I believe CoreGraphics uses an OS wide global glyph cache. With the Skia stuff we end up using both caches which is somewhat unfortunate.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> FWIW, I believe CoreGraphics uses an OS wide global glyph cache. With the
> Skia stuff we end up using both caches which is somewhat unfortunate.

Why do we use both?
(In reply to Eric Rahm [:erahm] from comment #10)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > FWIW, I believe CoreGraphics uses an OS wide global glyph cache. With the
> > Skia stuff we end up using both caches which is somewhat unfortunate.
> 
> Why do we use both?

CG has an OS wide cache that we can't quite get around. Skia has to ask OS X to render a specific glyph, then skia caches the result of the rendered glyph. When we ask OS X to render the glyph, OS X will cache that.
(In reply to Eric Rahm [:erahm] from comment #8)
> (In reply to Mason Chang [:mchang] from comment #7)
> > Just an FYI for Eric and Nicholas if they see a 10mb spike in memory on OS X.
> 
> Is this a per process regression or just in the parent process? Either way a
> 10MiB is a bad thing, it gets worse when you have multiple content processes
> which is an active concern (we're chipping away in KiB at this point).
> 
> So, I really think we should back this out and reevaluate whether or not we
> should just eat this regression.

Mason and I met to discuss this. We're planning on adding logic to scale back the cache size as the number of content processes grows and we'll add logic to purge the cache in response to low memory notifications.

As-is we can take the regression -- we think overall our memory use might be better with skia, I'll do some AWSY runs to compare CG and Skia.
See Also: → 1258781, 1258778
You need to log in before you can comment on or make changes to this bug.