Closed
Bug 1241865
Opened 9 years ago
Closed 6 years ago
Problem with canvas-2d-pixels memory reporting
Categories
(Core :: Graphics: Canvas2D, defect, P3)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
INACTIVE
People
(Reporter: jujjyl, Assigned: gw280)
References
(Depends on 1 open bug)
Details
(Whiteboard: [MemShrink:P3][gfx-noted])
Attachments
(3 files)
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•9 years ago
|
||
About:memory log at the one hour mark.
Updated•9 years ago
|
Whiteboard: [MemShrink]
Comment 2•9 years ago
|
||
The continuous nature of this leak is concerning. Is this a specific platform?
Flags: needinfo?(jujjyl)
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 3•9 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
Reporter | ||
Comment 4•9 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•9 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)
Updated•9 years ago
|
Whiteboard: [MemShrink:P1] → [MemShrink:P1][gfx-noted]
Comment 7•9 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.
Flags: needinfo?(bas)
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()?
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
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)
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.
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
Assignee | ||
Comment 15•8 years ago
|
||
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•8 years ago
|
Keywords: leave-open
Updated•8 years ago
|
Attachment #8790536 -
Flags: review?(nical.bugzilla) → review+
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
Comment 17•8 years ago
|
||
bugherder |
Comment 19•7 years ago
|
||
The original misreporting is removed, we're going to drop this from P1.
Whiteboard: [MemShrink:P1][gfx-noted] → [MemShrink:P3][gfx-noted]
Updated•7 years ago
|
Priority: -- → P3
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?
Flags: needinfo?(lsalzman)
Comment 21•6 years ago
|
||
Going to retire this bug since it has been inactive for so long and George is long gone.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(lsalzman)
Resolution: --- → INACTIVE
Updated•6 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•