Closed
Bug 1239151
Opened 8 years ago
Closed 8 years ago
7% - 10% TPS OS X talos regression with Skia Content
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mchang, Assigned: mchang)
References
Details
(Whiteboard: [talos_regression][gfx-noted])
Attachments
(1 file)
2.55 KB,
patch
|
lsalzman
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1207332 +++ See: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=451ff0ecf4b1&newProject=try&newRevision=c291279314ef&framework=1 Profiles: Base: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1614925221e3 Skia content: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae74f3fddc2b
Assignee | ||
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Whiteboard: [gfx-noted] → [talos_regression][gfx-noted]
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
Try looks good - https://hg.mozilla.org/try/rev/e976fd8362b0
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/99dcc91e0afd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 7•8 years ago
|
||
Just an FYI for Eric and Nicholas if they see a 10mb spike in memory on OS X.
Comment 8•8 years ago
|
||
(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.
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
(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?
Assignee | ||
Comment 11•8 years ago
|
||
(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.
Comment 12•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•