808266 - Add memory reporter for GrallocBufferActor

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

I don't entirely know what this is for, but we should add a memory reporter for it.  See bug 806029 comment 10 / attachment 677919 [details] [diff] [review].

Comment 1

[Nicholas Nethercote [inactive]]

Ugh, memory reporters for gfx suck :(

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

Is this enough?
Maybe we should add a better description of the memory type and its usage.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 680364 [details] [diff] [review]
patch

Do we need to count buffers allocated via ParamTraits<MagicGrallocBufferHandle>::Read()?  (I think this is called when de-marshalling a buffer received from IPC.)  Do these buffers live in shared memory -- are they different from the ones we allocate ourselves?

> Maybe we should add a better description of the memory type and its usage.

Yes; in fact, I think this will (non-fatally?) assert when used because (from nsIMemoryReporter):

> Reporters [...] must have [...] a
> description that is a sentence (i.e. starts with a capital letter and
> ends with a period, or similar).

I'd probably want answers to the following question in the description:

* Does the memory here get charged against a process's RSS/USS?  (I'd guess not.)
* Does the memory here get charged against the device's total free memory?  (That is, are we allocating into an existing reserved area?)  If the memory gets pulled out of the same main memory pool that processes use, I might not call it "GPU Memory" without further clarification.

We should also indicate (if it's factual) that this reporter may underestimate or overestimate the gralloc memory allocated by the process.  To get an accurate measurement, do X.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 680364 [details] [diff] [review]
patch

Review of attachment 680364 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +171,5 @@
> +  static bool registered;
> +  if (!registered) {
> +    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(GrallocBufferActor));
> +    registered = true;
> +  }

What's the lifetime of GrallocBufferActor, and how many of them are there?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 680364 [details] [diff] [review]
patch

There are a couple of higher-level issues with this patch, but I think
we can defer them to followups.

First is that the simple accounting here only tallies the memory that
the main b2g process asks the kernel to allocate.  This is the "real"
allocation across all processes, no overcounting, so it's the most
useful number.  But we'll report an allocation of "0 bytes" from each
subprocess if I'm reading things correctly.  That's a bit confusing
but probably better than trying to work out a scheme that charges the
memory to subprocesses in a sane way.

Second is that the allocation code runs on a non-main thread, but your
reporter runs on the main thread.  That means there's no guarantee
about the values returned by the reporter, and they even might be
garbage.  That's probably fine though.

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp

>+static size_t sCurrentAlloc;
>+static int64_t GetGrallocBufferActor() { return sCurrentAlloc; }

This name isn't particularly descriptive; how about
GetCurrentGrallocAllocationBytes()?  Be sure to add a note that it's
not "correct" to call this from a different thread than is doing the
gralloc allocations, caveat emptor.

>+NS_MEMORY_REPORTER_IMPLEMENT(GrallocBufferActor,

>+  "GPU Memory (allocated using Gralloc.h)")

jlebar can give better advice about where the long notes go, but this
one could use fleshing out a little bit.  Something like

 Special RAM that can be shared between processes and directly
 accessed by both the CPU and GPU.  Gralloc memory is usually a
 relatively precious resource, with much less available than generic
 RAM.  When it's exhausted, graphics performance can suffer.

>@@ -202,16 +236,20 @@ GrallocBufferActor::Create(const gfxIntS
> {
>   GrallocBufferActor* actor = new GrallocBufferActor();
>   *aOutHandle = null_t();
>   sp<GraphicBuffer> buffer(
>     new GraphicBuffer(aSize.width, aSize.height, aFormat, aUsage));
>   if (buffer->initCheck() != OK)
>     return actor;
> 
>+  size_t bpp = gfxASurface::BytePerPixelFromFormat(ImageFormatForPixelFormat(aFormat));
>+  actor->mAllocBytes = aSize.width * aSize.height * bpp;
>+  sCurrentAlloc += actor->mAllocBytes;
>+

Right, so we can't add this instrumentation without a lot more care.
This is going to result in gralloc being double-counted across the
master process and subprocesses.  This code also runs on the b2g main
thread, to support the system UI that's drawn by the main b2g process.
In that case, this code is a write/write hazard with the code that
runs on the compositor thread, which can corrupt this counter.

Let's just remove this for now.

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsGralloc.h b/gfx/layers/ipc/ShadowLayerUtilsGralloc.h

>   android::sp<GraphicBuffer> mGraphicBuffer;
>+
>+  size_t mAllocBytes;

My sloppy patch didn't do this (sorry), but we need to doc this.

Comment 6

[Andrea Marchesini [:baku]]

> * Does the memory here get charged against a process's RSS/USS?  (I'd guess
> not.)

I'm not able to answer to this question :)

> * Does the memory here get charged against the device's total free memory? 

Maybe Jones knows...

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pmem is allocated at boot time and doesn't show up as part of MemTotal.  I don't know if it's counted in RSS.  I would expect that it's not; otherwise one could compute total RAM allocations that exceeded MemTotal.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 681118 [details] [diff] [review]
patch b

Review of attachment 681118 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +155,5 @@
>    return gfxASurface::ContentFromFormat(ImageFormatForPixelFormat(aFormat));
>  }
>  
> +static size_t sCurrentAlloc;
> +static int64_t GetCurrentGrallocAllocationBytes() { return sCurrentAlloc; }

Nit: if you followed the convention from other reporters, this would probably be called "GetGrallocSize".

@@ +173,5 @@
> +{
> +  static bool registered;
> +  if (!registered) {
> +    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(GrallocBufferActor));
> +    registered = true;

I'll repeat my question from comment 4:  What's the lifetime of GrallocBufferActor, and how many of them are there?

I ask because here you're creating a memory reporter that will live until just before shutdown.  This isn't a problem if we only have one GrallocBufferActor.  But if we have more, you should save the reporter in a |mReporter| member and then unregister and NULL it in ~GrallocBufferActor().

Comment 10

[Andrea Marchesini [:baku]]

> I'll repeat my question from comment 4:  What's the lifetime of
> GrallocBufferActor, and how many of them are there?

I'll answer to this, as soon as possible.

Comment 11

[Andrea Marchesini [:baku]]

lifetimes goes from millisecs to many seconds.
About the number: we have more than 18 on average at startup time (homescreen + lockscreen probably) + opening browser.

Comment 12

[Justin Lebar (not reading bugmail)]

> I ask because here you're creating a memory reporter that will live until just before shutdown.  
> This isn't a problem if we only have one GrallocBufferActor.  But if we have more, you should save 
> the reporter in a |mReporter| member and then unregister and NULL it in ~GrallocBufferActor().

Actually, I don't understand what's the problem here, and how the number of GrallocBufferActors matters.  Why do we need to store the reporter as mReporter and unregister in the destructor?  That would require making one reporter per GrallocBufferActor...  (You noticed that the |registered| variable above is static, right?)

Comment 13

[Nicholas Nethercote [inactive]]

> (You noticed that the |registered| variable above is static, right?)

Nope!  So please ignore my objection.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch b - 2

This patch uses 1 reporter for each GrallocActor. This is nice because it shows the number of grallocactor used.
I don't make the previous patch obsolated because I think it's still a good and working patch.
Let me know what you prefer.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 681118 [details] [diff] [review]
patch b

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp

>+GrallocBufferActor::GrallocBufferActor()
>+: mAllocBytes(0)
>+{
>+  static bool registered;
>+  if (!registered) {
>+    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(GrallocBufferActor));
>+    registered = true;

This code is a bit subtle ... it just so happens that the first call here will always run on the main thread, but we shouldn't assume that.

From what jlebar tells me, this should be fine, but it's worth documenting.

We should also document that the gralloc reporter can return incorrect values because of race conditions.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 683116 [details] [diff] [review]
patch b - 2

We can create a pretty large number of gralloc buffers, and this code is relatively perf sensitive, so I prefer the previous patch.

Comment 17

[Andrea Marchesini [:baku]]

> >+  static bool registered;
> >+  if (!registered) {
> >+    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(GrallocBufferActor));
> >+    registered = true;
> 
> This code is a bit subtle ... it just so happens that the first call here
> will always run on the main thread, but we shouldn't assume that.

I can add an assert. Can it be enough?

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 683620 [details] [diff] [review]
patch c

Asserting this isn't strictly correct as it's not an invariant of this code.  However, it's fine for ensuring that something unexpected doesn't change our understanding of how this works.

Please add a comment above noting above.  No need to re-request review.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd1ad30ec92

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dd1ad30ec92