Closed
Bug 1258778
Opened 8 years ago
Closed 8 years ago
Purge the skia glyph cache when receiving a low memory notice
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mchang, Assigned: mchang)
References
Details
(Whiteboard: [gfx-noted][MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
3.59 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
From discussion with Eric, we should do this to improve memory usage in Skia.
Assignee | ||
Comment 1•8 years ago
|
||
Can you please just point me to what we should be listening for with an example? Thanks!
Flags: needinfo?(erahm)
Comment 2•8 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #1) > Can you please just point me to what we should be listening for with an > example? Thanks! The notification is 'memory-pressure'. We handle this in many places, there's a good example in the font cache [1]. I also noticed they expire entries in the cache [2], I'm not sure if that's doable with skia or not (or worth the effort). [1] https://dxr.mozilla.org/mozilla-central/rev/4037eb98974db1b1e0b5012c8a7f3a36428eaa11/gfx/thebes/gfxFont.cpp#127 [2] https://dxr.mozilla.org/mozilla-central/rev/4037eb98974db1b1e0b5012c8a7f3a36428eaa11/gfx/thebes/gfxFont.cpp#84-90
Flags: needinfo?(erahm)
Whiteboard: [gfx-noted] → [gfx-noted][MemShrink]
Assignee | ||
Comment 3•8 years ago
|
||
Looks like we should also just clear the skia glyph cache when layout also purges it's own font cache.
Attachment #8733565 -
Flags: review?(erahm)
Comment 4•8 years ago
|
||
Comment on attachment 8733565 [details] [diff] [review] Purge the skia glyph cache when we get a low mem notification Review of attachment 8733565 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine as long as we always use thebes.
Attachment #8733565 -
Flags: review?(erahm) → review+
Comment 5•8 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #3) > Looks like we should also just clear the skia glyph cache when layout also > purges it's own font cache. Are you going to follow up with that?
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #5) > (In reply to Mason Chang [:mchang] from comment #3) > > Looks like we should also just clear the skia glyph cache when layout also > > purges it's own font cache. > > Are you going to follow up with that? That's what this patch is doing. Layout has a font cache, so we purge Skia's at the same time.
Comment 7•8 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #6) > (In reply to Eric Rahm [:erahm] from comment #5) > > (In reply to Mason Chang [:mchang] from comment #3) > > > Looks like we should also just clear the skia glyph cache when layout also > > > purges it's own font cache. > > > > Are you going to follow up with that? > > That's what this patch is doing. Layout has a font cache, so we purge Skia's > at the same time. That's done a few places [1], not just in the observer. [1] https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-ref%3AgfxFontCache%3A%3AFlushShapedWordCaches%28%29
Assignee | ||
Comment 8•8 years ago
|
||
Found a better place to hook into the memory report! gfxPlatform has a memory-pressure observer already [1], which we hook into. Also, whenever gfxPlatform flushes the gfxFontCache [2], we flush Skia's cache as well. [1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp?from=gfxPlatform.cpp#466 [2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#1912
Attachment #8733565 -
Attachment is obsolete: true
Attachment #8733629 -
Flags: review?(erahm)
Comment 9•8 years ago
|
||
Comment on attachment 8733629 [details] [diff] [review] Purge the skia glyph cache when we get a low mem notification Review of attachment 8733629 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: gfx/thebes/gfxPlatform.cpp @@ +1304,5 @@ > void > +gfxPlatform::PurgeSkiaFontCache() > +{ > +#ifdef USE_SKIA > + // For content This seems like an incomplete statement.
Attachment #8733629 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Try looks good I think - https://treeherder.mozilla.org/#/jobs?repo=try&revision=adc24f5c6427
Updated•8 years ago
|
Whiteboard: [gfx-noted][MemShrink] → [gfx-noted][MemShrink:P2]
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/14fe71b46476
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•