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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

We currently have a hardcoded limit of 64MB. Let's make it more configurable.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: cache-pref.patch

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

That sounds reasonable, but we have to check how cut-the-rope game will behave in that case.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Attachment: preffable-cache.patch

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Also, gfxPlatform::InitializeSkiaCaches() never gets called.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add prefs for SkiaGL cache size

Hijacked this to finish it up

Comment 11

[George Wright (:gw280) (needinfo me!)]

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.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0759a746fb

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ce0759a746fb

Comment 15

[Preeti Raghunath(:Preeti)]

for memory enhancement being resolved.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

To be clear, after we land this for koi we still need to tweak these values to more reasonable things for b2g.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Although maybe we did some of that here.  snorp?

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 19

[George Wright (:gw280) (needinfo me!)]

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 20

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 21

[George Wright (:gw280) (needinfo me!)]

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".

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

As long as foolish != looking for a security hole, I'm good :)

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Attempt at unbitrotting:

https://hg.mozilla.org/releases/mozilla-aurora/rev/382f3133d961

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: 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.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=a0722d91c4f7

Comment 28

[Ed Morley [:emorley]]

(Not uplifting yet given pending review)

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Setting status-firefox26 to wontfix since this isn't going to be landing on mozilla-beta at this point.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/58faa33a2eb0