Open
Bug 1302380
Opened 5 years ago
Updated 2 years ago
Implement memory reporting in PersistentBufferProvider instead of CanvasRenderingContext2D
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
NEW
People
(Reporter: nical, Assigned: nical)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
18.00 KB,
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
Managing allocations has shifted from CanvasRenderingContext2D to the buffer provider which knows better how many buffers are allocated, whether they are in shared memory, etc. In bug 1241865 we disabled memory reporting in canvas because it was completely bogus. We should fix it and reenable it.
Assignee | ||
Comment 1•5 years ago
|
||
This is a lot simpler to get right than the previous way we tracking canvas allocations, and also accounts for double/triple buffering (previously we'd only count canvas.witdh * canvas*height * 4 bytes per canvas regardless of how many buffers we'd actually allocate for each canvas).
Attachment #8791549 -
Flags: review?(gwright)
Comment 2•5 years ago
|
||
Comment on attachment 8791549 [details] [diff] [review] Use the buffer provider to track canvas allocation Review of attachment 8791549 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure I'm a fan of passing in a pointer to a global static when creating the BufferProvider, but I'm ok with it if you are. ::: gfx/layers/Layers.cpp @@ +202,5 @@ > > already_AddRefed<PersistentBufferProvider> > LayerManager::CreatePersistentBufferProvider(const mozilla::gfx::IntSize &aSize, > + mozilla::gfx::SurfaceFormat aFormat, > + int64_t* aMemororyCounter) "Memory"
Attachment #8791549 -
Flags: review?(gwright) → review+
Assignee | ||
Comment 3•5 years ago
|
||
(In reply to George Wright (:gw280) (:gwright) from comment #2) > Comment on attachment 8791549 [details] [diff] [review] > Use the buffer provider to track canvas allocation > > Review of attachment 8791549 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure I'm a fan of passing in a pointer to a global static when > creating the BufferProvider, but I'm ok with it if you are. I don't have any emotional attachment to doing it this way but it was simple enough and not too risky. PersistentBufferProvider is intended to be independent of the type of layer that uses it, hence the indirection. I don't mind if someone comes up with a more elegant solution.
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d755ab4cbd Report canvas memory usage through the buffer provider. r=gw280
Backed out for timeouts reftest with e10s invariable-declaration-XX.html on Linux 32 bit: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2af231e0c321f010ebdf1a83eb028589225d27 Philor later mentioned that this is the same as bug 1300355 but on Linux, but be aware that this here was a permafailure (11/11 failed). Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e0d755ab4cbd6b2401fbbc592ef417f2fb0fda46 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35980893&repo=mozilla-inbound 02:18:52 INFO - REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/variable-declaration-24.html == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/support/color-green-ref.html 02:18:52 INFO - REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/variable-declaration-24.html | 1618 / 6895 (23%) 02:18:52 INFO - ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0xFFFB,name=???) Payload error: message could not be deserialized 02:18:52 INFO - REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/support/color-green-ref.html | 1618 / 6895 (23%) 02:18:52 INFO - [GFX1-]: Failed to create a valid ShmemTextureHost 02:18:52 INFO - ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0xFFFB,name=???) Payload error: message could not be deserialized 02:18:52 INFO - [GFX1-]: Failed to create a valid ShmemTextureHost 02:18:53 INFO - JavaScript error: chrome://reftest/content/reftest.jsm, line 1704: NS_ERROR_FAILURE: 02:24:23 ERROR - REFTEST ERROR | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/variable-declaration-24.html | application timed out after 330 seconds with no output 02:24:23 ERROR - REFTEST ERROR | Force-terminating active process(es). 02:24:23 INFO - REFTEST TEST-INFO | started process screentopng 02:24:23 INFO - REFTEST TEST-INFO | screentopng: exit 0 02:24:25 INFO - TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/variables/variable-declaration-24.html | application terminated with exit code 6
Flags: needinfo?(nical.bugzilla)
Comment 6•5 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #3) > > I don't have any emotional attachment to doing it this way but it was simple > enough and not too risky. PersistentBufferProvider is intended to be > independent of the type of layer that uses it, hence the indirection. I > don't mind if someone comes up with a more elegant solution. Yeah, I was going to suggest just externing it until I realised that the BufferProvider is not limited to CRC2D's usage. I'm ok with this, as I think anything else would add more complexity.
Backout by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/b401cb17167b Backed out changeset e0d755ab4cbd for timeouts reftest with e10s invariable-declaration-XX.html on Linux 32 bit. r=backout a=merge
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #5) > Backed out for timeouts reftest with e10s invariable-declaration-XX.html on > Linux 32 bit: This is very odd.
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 10•2 years ago
|
||
It's a 2 years old patch so I assume it'd need some work. I think the topic of the bug is still relevant today though (we don't count the double or triple buffering in the canvas allocation) so I think we should leave the bug open.
Flags: needinfo?(nical.bugzilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•