1302380 - Implement memory reporting in PersistentBufferProvider instead of CanvasRenderingContext2D

Status: NEW

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

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.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Use the buffer provider to track canvas allocation

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).

Comment 2

[George Wright (:gw280) (needinfo me!)]

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"

Comment 3

[Nicolas Silva [:nical]]

(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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d755ab4cbd
Report canvas memory usage through the buffer provider. r=gw280

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[George Wright (:gw280) (needinfo me!)]

(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.

Comment 7

[Pulsebot]

Backout by [e-mail]:
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

Comment 8

[Nicolas Silva [:nical]]

(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.

Comment 9

[Lee Salzman [:lsalzman]]

Nical, is this patch still viable?

Comment 10

[Nicolas Silva [:nical]]

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.