Open Bug 1302380 Opened 3 years ago Updated 9 months ago

Implement memory reporting in PersistentBufferProvider instead of CanvasRenderingContext2D

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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 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+
(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)
(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
(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)
Blocks: 1241865
Blocks: 1300592
Nical, is this patch still viable?
Flags: needinfo?(nical.bugzilla)
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.