[SkiaGL] Add prefs to control Skia's cache limits

RESOLVED FIXED in Firefox 27, Firefox OS v1.2

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: gw280, Assigned: snorp)

Tracking

(Depends on: 1 bug)

unspecified
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 wontfix, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [MemShrink:P1] )

Attachments

(2 attachments, 2 obsolete attachments)

We currently have a hardcoded limit of 64MB. Let's make it more configurable.
Created attachment 809332 [details] [diff] [review]
cache-pref.patch
Attachment #809332 - Flags: review?(snorp)
Comment on attachment 809332 [details] [diff] [review]
cache-pref.patch

Review of attachment 809332 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by review...

::: gfx/2d/DrawTargetSkia.cpp
@@ +103,5 @@
> +    sDynamicCacheInitialized = true;
> +  }
> +
> +  if (sDynamicCache) {
> +    int totalMemory = mozilla::hal::GetTotalSystemMemory();

uint32_t into int?  I understand it should fit because we return uint32_t/1024, but still...

@@ +109,5 @@
> +    if (!totalMemory) {
> +      sDynamicCache = false;
> +    }
> +
> +    cacheSize = totalMemory / 8;

This is ignored for if (!totalMemory), right?

@@ +117,5 @@
> +    static bool sCacheSizeLimitInitialized = false;
> +    static int sCacheSizeLimit = 0;
> +
> +    if (!sCacheSizeLimitInitialized) {
> +      sCacheSizeLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-size", 64);

Check for <0?

Also, I guess we're ok without checking the ceiling, because this is just the maximum cache size, so if they specify a very large number, it doesn't matter unless they can actually fill it.

Still, what's the cost of a call to GetTotalSystemMemory() all the time?  Because we could then enforce "no more than X% of total memory can be dedicated to the cache" or something like that.

@@ +141,3 @@
>    }
>  
> +  printf_stderr("Determined SkiaGL cache limits: Size %i, Items: %i\n", cacheSize, sCacheItemLimit);

We want this left in?
Duplicate of this bug: 918978
In bug 918978 we found that even 16MB is too large of a cache on hamachi, which has 256MB of total memory. We need to just disable SkiaGL in that case.

Updated

4 years ago
Blocks: 918978
That sounds reasonable, but we have to check how cut-the-rope game will behave in that case.
Created attachment 814469 [details] [diff] [review]
preffable-cache.patch
Attachment #809332 - Attachment is obsolete: true
Attachment #809332 - Flags: review?(snorp)
Attachment #814469 - Flags: review?(snorp)
Comment on attachment 814469 [details] [diff] [review]
preffable-cache.patch

Review of attachment 814469 [details] [diff] [review]:
-----------------------------------------------------------------

Ok with nits

::: gfx/thebes/gfxPlatform.cpp
@@ +836,5 @@
> +    if (usingDynamicCache) {
> +      uint32_t totalMemory = mozilla::hal::GetTotalSystemMemory();
> +
> +      if (totalMemory) {
> +        cacheSizeLimit = totalMemory / 8;

This would mean on our Tegras, which I believe are 1GB, we would choose 128MB. I'm pretty sure this will cause us to run out of ram currently. I guess change to "totalMemory / 16"?

::: hal/Hal.cpp
@@ +1193,5 @@
> +
> +  if (!sTotalMemoryObtained) {
> +    sTotalMemoryObtained = true;
> +
> +    FILE* fd = fopen("/proc/meminfo", "r");

Please file a followup bug to get platform-dependent impls of this. The current code only works on Linux/Android/B2G.
Attachment #814469 - Flags: review?(snorp) → review+
Also, gfxPlatform::InitializeSkiaCaches() never gets called.
It may be worth separating out the heuristic for computing the "automatic limit" from the rest of this patch, so that we can land something while we're sorting out exactly what we want to do?  It may be that we put a little table in there, rather than just try to take a same percentage all the time, or a few other things, but if we have a pref we can get some more empirical data to support a heuristic?
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]
Blocks: 927254
blocking-b2g: --- → koi?
Created attachment 819939 [details] [diff] [review]
Add prefs for SkiaGL cache size

Hijacked this to finish it up
Assignee: gwright → snorp
Attachment #814469 - Attachment is obsolete: true
Attachment #819939 - Flags: review?(gwright)
Comment on attachment 819939 [details] [diff] [review]
Add prefs for SkiaGL cache size

Review of attachment 819939 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm other than the comments

::: gfx/thebes/gfxPlatform.cpp
@@ +822,5 @@
> +  if (UseAcceleratedSkiaCanvas()) {
> +    bool usingDynamicCache = Preferences::GetBool("gfx.canvas.skiagl.dynamic-cache", false);
> +
> +    int cacheItemLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-items", 256);
> +    int cacheSizeLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-size", 64);

These should probably match the static ints in DrawTargetSkia's default values, even if it does override in the end. Less confusion.

@@ +831,5 @@
> +    if (usingDynamicCache) {
> +      uint32_t totalMemory = mozilla::hal::GetTotalSystemMemory();
> +
> +      if (totalMemory <= 256*1024*1024) {
> +        // We need a very minimal cache on 256 meg devices

I really vote against this. If we've only got a 2MB texture cache to be shared between all canvases, we should just give up and switch to software. No GPU hotness for 256MB.
Attachment #819939 - Flags: review?(gwright) → review+
(In reply to George Wright (:gw280) from comment #11)
> Comment on attachment 819939 [details] [diff] [review]
> Add prefs for SkiaGL cache size
> 
> Review of attachment 819939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm other than the comments
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +822,5 @@
> > +  if (UseAcceleratedSkiaCanvas()) {
> > +    bool usingDynamicCache = Preferences::GetBool("gfx.canvas.skiagl.dynamic-cache", false);
> > +
> > +    int cacheItemLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-items", 256);
> > +    int cacheSizeLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-size", 64);
> 
> These should probably match the static ints in DrawTargetSkia's default
> values, even if it does override in the end. Less confusion.

Changed them both to 96MB.

> 
> @@ +831,5 @@
> > +    if (usingDynamicCache) {
> > +      uint32_t totalMemory = mozilla::hal::GetTotalSystemMemory();
> > +
> > +      if (totalMemory <= 256*1024*1024) {
> > +        // We need a very minimal cache on 256 meg devices
> 
> I really vote against this. If we've only got a 2MB texture cache to be
> shared between all canvases, we should just give up and switch to software.
> No GPU hotness for 256MB.

I thought about that too, but we really do get a performance improvement on some stuff with just a 2MB cache. On hamachi, Poppit went from about unplayable <10fps to almost 30 for me, so I think we should keep this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0759a746fb
https://hg.mozilla.org/mozilla-central/rev/ce0759a746fb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
for memory enhancement being resolved.
blocking-b2g: koi? → koi+
To be clear, after we land this for koi we still need to tweak these values to more reasonable things for b2g.
Although maybe we did some of that here.  snorp?
Yes, it will be easier to play with the values with this landed.  We may still conclude that we don't want SkiaGL at all on 1.2, in which case we wouldn't have needed it, but even if that ends up being the case, it's still better to do this.
But, if it does land, my understanding is that we don't need to tweak the values further, it is set to "dynamic cache" by default.
Yes, it's set to by default set the texture cache limits to 1/16th of the total system memory, with the exception that if we're a 256MB device or smaller, then set the limit to 2MB. These are on the more conservative side of things, so if anything I suspect we'd be increasing the limits rather than decreasing them.
Comment on attachment 819939 [details] [diff] [review]
Add prefs for SkiaGL cache size

Review of attachment 819939 [details] [diff] [review]:
-----------------------------------------------------------------

Wonder if it'd have been useful to have the variable names explicitly convey when the values are in bytes vs. MB or KB...

::: gfx/thebes/gfxPlatform.cpp
@@ +822,5 @@
> +  if (UseAcceleratedSkiaCanvas()) {
> +    bool usingDynamicCache = Preferences::GetBool("gfx.canvas.skiagl.dynamic-cache", false);
> +
> +    int cacheItemLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-items", 256);
> +    int cacheSizeLimit = Preferences::GetInt("gfx.canvas.skiagl.cache-size", 64);

Do you need to protect against non-positive values here?  Also, does it make sense to only get cacheSizeLimit if we're not using dynamic cache?  Guess the code is cleaner if we always get it.
If we're dynamic we'll never bother honouring the cache size pref anyway, so I don't think there's any point in retrieving it (less IO is always better, right?). Might be worth switching to doing that in that case.

Negative values: I don't see it as that important as it'd only be a guard against a user being foolish, but it might be worth having a negative value meaning "unbounded texture cache".
As long as foolish != looking for a security hole, I'm good :)
This will need an updated patch for aurora/koi uplift:

[/c/src-gecko/aurora]$ transplant ce0759a746fb
searching for changes
applying ce0759a746fb
patching file b2g/app/b2g.js
Hunk #1 FAILED at 808
1 out of 1 hunks FAILED -- saving rejects to file b2g/app/b2g.js.rej
patching file gfx/2d/DrawTargetSkia.cpp
Hunk #1 FAILED at 70
1 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetSkia.cpp.rej
patching file gfx/2d/DrawTargetSkia.h
Hunk #2 FAILED at 126
1 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetSkia.h.rej
patching file gfx/thebes/gfxPlatform.cpp
Hunk #1 FAILED at 56
Hunk #2 FAILED at 271
2 out of 3 hunks FAILED -- saving rejects to file gfx/thebes/gfxPlatform.cpp.rej
patching file hal/moz.build
Hunk #1 FAILED at 44
Hunk #2 FAILED at 79
Hunk #3 FAILED at 112
3 out of 3 hunks FAILED -- saving rejects to file hal/moz.build.rej
patch failed to apply
Whiteboard: [MemShrink:P1] → [MemShrink:P1] [needs-aurora-patch]
Attempt at unbitrotting:

https://hg.mozilla.org/releases/mozilla-aurora/rev/382f3133d961
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
Whiteboard: [MemShrink:P1] [needs-aurora-patch] → [MemShrink:P1]
Backed out for causing Android reftest failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7090e4028a66

https://tbpl.mozilla.org/php/getParsedLog.php?id=29752631&tree=Mozilla-Aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=29752478&tree=Mozilla-Aurora
status-b2g-v1.2: fixed → affected
status-firefox26: fixed → affected
Keywords: branch-patch-needed
Flags: needinfo?(snorp)
Created attachment 823339 [details] [diff] [review]
Enable dynamic SkiaGL cache sizing on Android

Ryan's patch looks fine, but we need to enable the dynamic cache on Android too, otherwise we run out of ram during reftests because the cache is 96MB. On m-c we disable SkiaGL for reftests (sigh), so we don't see this issue.
Attachment #823339 - Flags: review?(gwright)
Flags: needinfo?(snorp)
Try run here: https://tbpl.mozilla.org/?tree=Try&rev=a0722d91c4f7
(Not uplifting yet given pending review)
We probably just want to uplift the patch from 907351, I guess, to keep parity with m-c. My other patch is probably still necessary, though.
Depends on: 907351
(Reporter)

Updated

4 years ago
Attachment #823339 - Flags: review?(gwright) → review+
Setting status-firefox26 to wontfix since this isn't going to be landing on mozilla-beta at this point.
status-firefox26: affected → wontfix
Keywords: branch-patch-needed
So here's the thing - now that b2g26 has branched, we don't need to worry about Android anymore as no Android builds/tests are made on that branch. How does that affect what we want to do here? Just re-land what I previously pushed to Aurora?
Flags: needinfo?(snorp)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31)
> So here's the thing - now that b2g26 has branched, we don't need to worry
> about Android anymore as no Android builds/tests are made on that branch.
> How does that affect what we want to do here? Just re-land what I previously
> pushed to Aurora?

Yeah, that's fine.
Flags: needinfo?(snorp)
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/58faa33a2eb0
status-b2g-v1.2: affected → fixed
Depends on: 1280514
You need to log in before you can comment on or make changes to this bug.