I was recently pointed to a testcase (unfortunately not public), which is a webapp that uses 2d canvas extensively. On this webapp, we end up with a non-hardware-accelerated canvas for the main big canvas it uses for drawing. The reason for that is that it creates a bunch of small canvases during startup, so we end up demoting the big canvas. I logged just the sizes of the canvases getting demoted and I see 20 or 30 116x144 canvases (note that this is _just_ over our heuristic of area >= 128*128 for acceleration!) get demoted before the main 2946x1966 canvas does. Then I see a few more small canvases also get demoted, with sizes like 160x110 and 166x125. It's hard to figure out how to improve this heuristic in general, since we're trying to guess which canvases _will_ be used in the future. But we should maybe consider re-promoting canvases that get a lot of activity. Or something.

We can probably start by making it so that if width or height are < 128, we don't do it in GL, as well as checking the area to ensure we're smaller than 128x128. http://searchfox.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1461

Right, it's hard to tell whether that would be better or worse in general. I _think_ it would help my specific testcase, but in general it's hard to say...

Milan, Jeff - how much do/should we care about SkiaGL? I'm starting to think that maybe we should consider sunsetting it, as I'm not sure whether the gains it gives us is worth the various complexities we have.

In the testcase in question, with the hardware accel disabled drawImage starts gating everything, for what it's worth...

My suggestion is that we try to have our heuristics match chrome. I think the performance compatibility advantage outweighs any innovation we'd have here.

Matching Chrome is a good approach. I didn't fully understand the description - are we actually demoting the large canvas?

> are we actually demoting the large canvas? Yes, we are. That is "the problem" for this particular testcase...

Interesting. There shouldn't be any connection between the demoting and what happens to other canvases, so either we have a bug, or the large canvas makes more than four GetImageData/PutImageData calls in the first 30 frames, and fewer than that many calls to DrawImage. Can we verify with the authors that they do make those calls early on?

> There shouldn't be any connection between the demoting and what happens to other canvases There is, because of the global cap on total number of live accelerated canvases. See CanvasRenderingContext2D::DemoteOldestContextIfNecessary() which is called when we try to create a new accelerated canvas. That's why we're getting demoted: the page creates the large canvas, then creates > 64 small scratch canvases for some things its doing, all without us running a CC and collecting any of this stuff, then the big canvas gets demoted.

Got it. Forgot about that. Not the best, I agree. I'm also pretty sure that there is a bug and that we can end up with demoting even when we don't have to (e.g., before we reach 64); I'll take a quick look.

Does this help the test case at all? Ignoring how it's implemented :)

I'm not sure that having an upper limit makes sense on non-mobile platforms, for what it's worth.

Sure, at the minimum, we should move that limit to a pref, have <=0 mean unlimited, and set the pref differently for different platforms.

> Does this help the test case at all? Ignoring how it's implemented :) Yes. When applied on top of the patches in bug 1335139, it takes the testcase from about 23fps to about 27fps.

Comment on attachment 8832265 [details] [diff] [review] 0001-Bug-1335145-Only-limit-accelerated-canvases-for-mobi.patch Review of attachment 8832265 [details] [diff] [review]: ----------------------------------------------------------------- We have two options - this is a startup ("Once") preference in which case I'd like to see an optimization for the <=0 case in not even collecting/adding/removing/tracking demotable contexts - we will never actually do it. Perhaps not a lot of overhead, but some, and there would be no need for it. If this is a "live" preference, where the users can change the value at run time, then we shouldn't make that optimization, but I think I like the startup "Once" option better. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1420,4 @@ > > void > CanvasRenderingContext2D::DemoteOldestContextIfNecessary() > + int32_t maxContexts = gfxPrefs::CanvasAcceleratedAzureLimit(); Given that this is a "once" preference, we may as well make this static as well. No need to keep asking for the new value, it won't change during the run.

George, if it makes sense, feel free to combine this with your patch(es) and get it reviewed together.

Huh, thought I'd uploaded this for review. Guess I hadn't.

Comment on attachment 8833426 [details] [diff] [review] 0001-Bug-1335145-Only-limit-accelerated-canvases-for-mobi.patch Review of attachment 8833426 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1049,5 @@ > // Initialize our static variables. > uint32_t CanvasRenderingContext2D::sNumLivingContexts = 0; > DrawTarget* CanvasRenderingContext2D::sErrorTarget = nullptr; > +static bool sMaxContextsInitialized = false; > +static int32_t sMaxContexts; Nit: I'm a bit nervous about sMaxContexts not being initialized here (to 0?) given that it's used in a static function, but initialized in a member function. It's probably the case that we wouldn't call one of those static methods without a canvas constructed, but still. I'd initialize this to some value (-1 or 0), and put MOZ_ASSERT(sMaxContextsInitialized) in the places where we're using the value - it would be nice to catch any scenarios that trigger it.

(In reply to Milan Sreckovic [:milan] from comment #20) > Comment on attachment 8833426 [details] [diff] [review] > 0001-Bug-1335145-Only-limit-accelerated-canvases-for-mobi.patch > > Review of attachment 8833426 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with nits. > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +1049,5 @@ > > // Initialize our static variables. > > uint32_t CanvasRenderingContext2D::sNumLivingContexts = 0; > > DrawTarget* CanvasRenderingContext2D::sErrorTarget = nullptr; > > +static bool sMaxContextsInitialized = false; > > +static int32_t sMaxContexts; > > Nit: I'm a bit nervous about sMaxContexts not being initialized here (to 0?) > given that it's used in a static function, but initialized in a member > function. It's probably the case that we wouldn't call one of those static > methods without a canvas constructed, but still. I'd initialize this to > some value (-1 or 0), and put MOZ_ASSERT(sMaxContextsInitialized) in the > places where we're using the value - it would be nice to catch any scenarios > that trigger it. Aren't statics always initialised to 0? Anyway, I can do this.

>+ if (!sMaxContextInitialized) { Won't compile; missing 's' from "Contexts". >+ sMaxContexts = gfxPrefs::CanvasAcceleratedAzureLimit(); Won't compile either; that's not what this function is called. >CanvasRenderingContext2D::DemoteOldestContextIfNecessary() > if (sMaxContexts <= 0) { Won't compile; missing '{' for function body.... > if (contexts.size() < sMaxContexts) Warns about comparing "unsigned long" to "int32_t"; may die in automation due to -Werror.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22) > >+ if (!sMaxContextInitialized) { > > Won't compile; missing 's' from "Contexts". > > >+ sMaxContexts = gfxPrefs::CanvasAcceleratedAzureLimit(); > > Won't compile either; that's not what this function is called. > > >CanvasRenderingContext2D::DemoteOldestContextIfNecessary() > > if (sMaxContexts <= 0) { > > Won't compile; missing '{' for function body.... > > > if (contexts.size() < sMaxContexts) > > Warns about comparing "unsigned long" to "int32_t"; may die in automation > due to -Werror. Yeah, I guess it's obvious that I put this together quickly without compiling it :) Anyway, yes, will obviously fix the compiler issues before landing.

Pushed by gwright@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/851149e5e759 Only limit accelerated canvases for mobile r=milan