Closed
Bug 920160
Opened 11 years ago
Closed 11 years ago
[SkiaGL] Add prefs to control Skia's cache limits
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: gw280, Assigned: snorp)
References
(Depends on 1 open bug)
Details
(Whiteboard: [MemShrink:P1] )
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
We currently have a hardcoded limit of 64MB. Let's make it more configurable.
Comment 2•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
That sounds reasonable, but we have to check how cut-the-rope game will behave in that case.
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #809332 -
Attachment is obsolete: true
Attachment #809332 -
Flags: review?(snorp)
Attachment #814469 -
Flags: review?(snorp)
Assignee | ||
Comment 7•11 years ago
|
||
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+
Comment 9•11 years ago
|
||
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]
![]() |
||
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → koi?
Assignee | ||
Comment 10•11 years ago
|
||
Hijacked this to finish it up
Assignee: gwright → snorp
Attachment #814469 -
Attachment is obsolete: true
Attachment #819939 -
Flags: review?(gwright)
Reporter | ||
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0759a746fb
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce0759a746fb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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?
Comment 18•11 years ago
|
||
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.
Reporter | ||
Comment 19•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 21•11 years ago
|
||
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 23•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [MemShrink:P1] → [MemShrink:P1] [needs-aurora-patch]
Comment 24•11 years ago
|
||
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]
Comment 25•11 years ago
|
||
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
Updated•11 years ago
|
Flags: needinfo?(snorp)
Assignee | ||
Comment 26•11 years ago
|
||
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)
Assignee | ||
Comment 27•11 years ago
|
||
Try run here: https://tbpl.mozilla.org/?tree=Try&rev=a0722d91c4f7
Assignee | ||
Comment 29•11 years ago
|
||
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.
Reporter | ||
Updated•11 years ago
|
Attachment #823339 -
Flags: review?(gwright) → review+
Comment 30•11 years ago
|
||
Setting status-firefox26 to wontfix since this isn't going to be landing on mozilla-beta at this point.
Updated•11 years ago
|
Keywords: branch-patch-needed
Comment 31•11 years ago
|
||
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)
Assignee | ||
Comment 32•11 years ago
|
||
(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)
You need to log in
before you can comment on or make changes to this bug.
Description
•