Jukka Jylänki Reporter
	Description • 8 years ago

Running the Voxatron demo in the emunittest 0.5 suite repeatedly, there seems to be a small running growth of canvas-2d-pixels memory after each run. About:memory reports the following:

after 0 runs: 0.00 MB ── canvas-2d-pixels
after 1 run:  1.62 MB ── canvas-2d-pixels
after 2 runs: 3.23 MB ── canvas-2d-pixels
after 3 runs: 4.85 MB ── canvas-2d-pixels
after 4 runs: 6.47 MB ── canvas-2d-pixels
after 5 runs: 8.09 MB ── canvas-2d-pixels

which is a linear increase of 1.62MB of memory in canvas 2d per run. 

Leaving the test running in sequential torture test mode, about:memory shows the following:

after 15 minutes: 25.88 MB ── canvas-2d-pixels
after about an hour: 590.36 MB ── canvas-2d-pixels

Attached DMD log after the one hour run, but the memory consumed by canvas-2d-pixels is not shown there. Is the memory taken by canvas-2d-pixels GPU memory?

	Jukka Jylänki Reporter
	Comment 1 • 8 years ago

About:memory log at the one hour mark.

	Eric Rahm [:erahm]
	Comment 2 • 8 years ago

The continuous nature of this leak is concerning. Is this a specific platform?

	Eric Rahm [:erahm]
	Comment 3 • 8 years ago

It's also possible this is just a reporting error, that value comes from a global counter [1] rather than an actual measurement. We increment it in |CanvasRenderingContext2D::EnsureTarget| [2] and decrement it in |CanvasRenderingContext2D::Reset| [3].

Someone on the graphics side probably needs to audit that logic.

[1] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/canvas/CanvasRenderingContext2D.cpp#177
[2] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/canvas/CanvasRenderingContext2D.cpp#1459
[3] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/canvas/CanvasRenderingContext2D.cpp#1054

	Jukka Jylänki Reporter
	Comment 4 • 8 years ago

This was tested on Windows. I don't know of other platforms. If there is a Mozilla developer who wants to take this on, I can send the emunittest suite to him, please ping me directly via email. The suite is unfortunately not public, so can't post it here.

The fact that the memory (assuming it is supposed to be CPU side memory and not GPU memory) is not shown in the attached DMD log above(?) would suggest that it's a possibility that it is a bug with miscounting allocations.

	Guilherme Lima
	Comment 5 • 8 years ago

On https://bugzilla.mozilla.org/show_bug.cgi?id=1024277#c6 I reported a problem with the reported value on about:memory for canvas-2d-pixels

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 6 • 8 years ago

Bas, is this bug 1252677 or that other "canvas holds on to things longer now, until destroyed" thing, or do you think we're miscounting the memory?

	Bas Schouten (:bas.schouten)
	Comment 7 • 8 years ago

(In reply to Milan Sreckovic [:milan] from comment #6)
> Bas, is this bug 1252677 or that other "canvas holds on to things longer
> now, until destroyed" thing, or do you think we're miscounting the memory?

This ought to be a miscounting issue (or a leaking actual canvas objects). The other additional mem usage you're referring to won't show up in there.

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 8 years ago

I noticed this recently as well.  The canvas-2d-pixels line in about:memory basically reflects gCanvasAzureMemoryUsed.  This is set in EnsureTarget():

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1511

And decremented in Reset();

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1107

The reset decrement only happens if mTarget is value, though:

  if (mTarget && IsTargetValid() && !mDocShell) {
    gCanvasAzureMemoryUsed -= mWidth * mHeight * 4;
  }

It seems there are many places in the code, however, where mTarget can get nulled out or changed.  In particular, we clear the mTarget and call EnsureTarget() again in SwitchRenderingMode().  This seems almost certain to double-count memory:

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1294

Bas, do you think its worth reducing gCanvasAzureMemoryUsed there just like we do in Reset()?

	Bas Schouten (:bas.schouten)
	Comment 9 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #8)
> I noticed this recently as well.  The canvas-2d-pixels line in about:memory
> basically reflects gCanvasAzureMemoryUsed.  This is set in EnsureTarget():
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp#1511
> 
> And decremented in Reset();
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp#1107
> 
> The reset decrement only happens if mTarget is value, though:
> 
>   if (mTarget && IsTargetValid() && !mDocShell) {
>     gCanvasAzureMemoryUsed -= mWidth * mHeight * 4;
>   }
> 
> It seems there are many places in the code, however, where mTarget can get
> nulled out or changed.  In particular, we clear the mTarget and call
> EnsureTarget() again in SwitchRenderingMode().  This seems almost certain to
> double-count memory:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp#1294
> 
> Bas, do you think its worth reducing gCanvasAzureMemoryUsed there just like
> we do in Reset()?

Yeah, this code is being touched by Nical at the moment though, let's fix it after he's done with it.

	Eric Rahm [:erahm]
	Comment 10 • 8 years ago

Bas, can we revisit this now?

	Bas Schouten (:bas.schouten)
	Comment 11 • 8 years ago

(In reply to Eric Rahm [:erahm] from comment #10)
> Bas, can we revisit this now?

We probably should, however things have changed someone in how canvas works since canvas-2d-pixels was last used, and it makes it a little tricky to determine how to do this. I feel we'd be better off changing this metric to be a measurement of the 'potential' usage of all canvasses, i.e. we'll increment/decrement appropriately on resize/creation/destruction of the canvas object regardless of whether a DT is actually present or not.

	Mike Hommey [:glandium]
	Comment 12 • 8 years ago

If it's not going to be fixed in a short time frame, can we just remove it from about:memory? All it's achieving is puzzling people. Like when it's larger than resident or even vsize, like it currently is on my machine (13.7G canvas-2d-pixels in the content process)

	Nicholas Nethercote [inactive]
	Comment 13 • 8 years ago

glandium just showed me a memory report with the following in the content process:

> 13,784.55 MB ── canvas-2d-pixels
>      0.05 MB ── gfx-surface-image
>      0.00 MB ── gfx-surface-xlib
>      0.00 MB ── gfx-textures
>      0.00 MB ── gfx-textures-peak
>      0.00 MB ── gfx-tiles-waste
>            0 ── ghost-windows
>  2,014.45 MB ── heap-allocated
>      1.00 MB ── heap-chunksize
>  2,209.00 MB ── heap-mapped
>            2 ── host-object-urls
>      0.67 MB ── imagelib-surface-cache-estimated-locked
>      3.46 MB ── imagelib-surface-cache-estimated-total
>            0 ── imagelib-surface-cache-overflow-count
>     12.85 MB ── js-main-runtime-temporary-peak
>       10,613 ── page-faults-hard
>  200,756,001 ── page-faults-soft
>  2,125.81 MB ── resident
>  2,642.23 MB ── resident-peak
>  2,064.80 MB ── resident-unique
>     34.09 MB ── shmem-allocated
>     34.78 MB ── shmem-mapped
>      0.00 MB ── system-heap-allocated
>  3,592.54 MB ── vsize

canvas-2d-pixels is 10 GiB more than even vsize, which is ridiculous. The measurement is clearly broken and untrustworthy. If we can't get this fixed then I suggest we disable the reporter because it's causing harm than good at the moment.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 14 • 8 years ago

Agreed - lets disable this, and try to fix it and enable it.  George, can you look at this?

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 15 • 8 years ago

As per Milan's comment, disable this for now. I'm currently investigating why it's busted.

	Pulsebot
	Comment 16 • 8 years ago

Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e86a4830af9
Disable canvas-2d-pixels memory reporter as it currently reports inaccurate values. r=nical

	Carsten Book [:Tomcat]
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0e86a4830af9

	Paul Graphonium
	Comment 18 • 7 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=454084#c8

	Eric Rahm [:erahm]
	Comment 19 • 7 years ago

The original misreporting is removed, we're going to drop this from P1.

	BugBot [:suhaib / :marco/ :calixte]
	Comment 20 • 6 years ago

The leave-open keyword is there and there is no activity for 6 months.
:lsalzman, maybe it's time to close this bug?

	Lee Salzman [:lsalzman]
	Comment 21 • 6 years ago

Going to retire this bug since it has been inactive for so long and George is long gone.