Closed Bug 1258778 Opened 4 years ago Closed 4 years ago

Purge the skia glyph cache when receiving a low memory notice

Categories

(Core :: Graphics, defect)

defect
Not set

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)

From discussion with Eric, we should do this to improve memory usage in Skia.
Can you please just point me to what we should be listening for with an example? Thanks!
Flags: needinfo?(erahm)
See Also: → 1239151
(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]
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 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+
(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?
(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.
(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
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 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+
Whiteboard: [gfx-noted][MemShrink] → [gfx-noted][MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/14fe71b46476
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.