Problem with canvas-2d-pixels memory reporting

NEW
Assigned to

Status

()

Core
Canvas: 2D
P3
normal
2 years ago
11 months ago

People

(Reporter: Jukka Jylänki, Assigned: gw280)

Tracking

(Depends on: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3][gfx-noted])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8710986 [details]
results_voxatron_onehour.zip

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?
(Reporter)

Comment 1

2 years ago
Created attachment 8710987 [details]
unified-memory-report-1453469124-13988.json.gz

About:memory log at the one hour mark.
Whiteboard: [MemShrink]
The continuous nature of this leak is concerning. Is this a specific platform?
Flags: needinfo?(jujjyl)
Whiteboard: [MemShrink] → [MemShrink:P1]
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
(Reporter)

Comment 4

2 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.
Flags: needinfo?(jujjyl)

Comment 5

2 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
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?
Flags: needinfo?(bas)
Whiteboard: [MemShrink:P1] → [MemShrink:P1][gfx-noted]
(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.
Flags: needinfo?(bas)

Comment 8

2 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()?
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
Bas, can we revisit this now?
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
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)
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.
Agreed - lets disable this, and try to fix it and enable it.  George, can you look at this?
Assignee: nobody → gwright
Flags: needinfo?(gwright)
Summary: Memory leak in canvas-2d-pixels → Problem with canvas-2d-pixels memory reporting
Created attachment 8790536 [details] [diff] [review]
0001-Bug-1241865-Disable-canvas-2d-pixels-memory-reporter.patch

As per Milan's comment, disable this for now. I'm currently investigating why it's busted.
Flags: needinfo?(gwright)
Attachment #8790536 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Keywords: leave-open

Updated

2 years ago
Attachment #8790536 - Flags: review?(nical.bugzilla) → review+

Updated

2 years ago
See Also: → bug 1302380

Comment 16

2 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
See Also: → bug 1300592
Depends on: 1302380
The original misreporting is removed, we're going to drop this from P1.
Whiteboard: [MemShrink:P1][gfx-noted] → [MemShrink:P3][gfx-noted]
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.