973777 - Breaking down the information of GrallocReporter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Solomon Chiu [:schiu]]

The number which reported by GrallocReporter now is grand total of all GraphicBuffer consumption, which is hard to tell the information of individual GraphicBuffer. The reported number maybe can break down to the consumption of each allocated GraphicBuffer. Furthermore, the dimension and bpp can be presented in memory report, such that we can roughly know what a specific graphicbuffer is refering to.

Comment 1

[Peter Chang[:pchang]]

Provide the detail of gralloc buffer usage is good. 
How to use this memory tool is more important. As we talked during the meeting, please create bug for one click memory usage snapshot for memory analysis, like the following
a. run the tool to capture current memory report
b. reproduce something
c. stop the tool to capture another new memory report
d. display the difference between these two reports

Comment 2

[Solomon Chiu [:schiu]]

Attachment: WIP: Gralloc reporter

Due to the refactoring work in bug#959089, GrallocReport has been move to SharedBufferManagerParent.cpp from ShadowLayerUtilsGralloc.cpp. This patch is base on SharedBufferManagerParent and leverage the pid information to present the broken down information of gralloc usage.

Comment 3

[Solomon Chiu [:schiu]]

Comment on attachment 8419267 [details] [diff] [review]
WIP: Gralloc reporter

>commit 6d4c87dacf5995fd80b145ee19b51cbdf99d1fbd
>Author: Solomon Chiu <[e-mail]>
>Date:   Thu May 8 14:53:00 2014 +0800
>
>    Gralloc reporter refine, draft-1
>
>diff --git a/gfx/layers/ipc/SharedBufferManagerParent.cpp b/gfx/layers/ipc/SharedBufferManagerParent.cpp
>index 8e0fbdf..1f782d2 100644
>--- a/gfx/layers/ipc/SharedBufferManagerParent.cpp
>+++ b/gfx/layers/ipc/SharedBufferManagerParent.cpp
>@@ -13,74 +13,114 @@
> #include "base/thread.h"
> #include "mozilla/ipc/MessageChannel.h" // for MessageChannel, etc
> #include "mozilla/ipc/ProtocolUtils.h"
> #include "mozilla/ipc/Transport.h"      // for Transport
> #include "nsIMemoryReporter.h"
> #ifdef MOZ_WIDGET_GONK
> #include "ui/PixelFormat.h"
> #endif
> #include "nsThreadUtils.h"
> 
>+#include "nsPrintfCString.h"
>+
> using namespace mozilla::ipc;
> #ifdef MOZ_WIDGET_GONK
> using android::sp;
> using android::GraphicBuffer;
> #endif
> using std::map;
> 
> namespace mozilla {
> namespace layers {
> 
>+map<base::ProcessId, SharedBufferManagerParent* > SharedBufferManagerParent::sManagers;
>+StaticAutoPtr<Monitor> SharedBufferManagerParent::sManagerMonitor;
>+int SharedBufferManagerParent::sBufferKey = 0;
>+
> class GrallocReporter MOZ_FINAL : public nsIMemoryReporter
> {
> public:
>   NS_DECL_ISUPPORTS
> 
>-  GrallocReporter()
>-  {
>-#ifdef DEBUG
>-    // There must be only one instance of this class, due to |sAmount|
>-    // being static.  Assert this.
>-    static bool hasRun = false;
>-    MOZ_ASSERT(!hasRun);
>-    hasRun = true;
>-#endif
>-  }
>-
>   NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
>                             nsISupports* aData)
>   {
>-    return MOZ_COLLECT_REPORT(
>-      "gralloc", KIND_OTHER, UNITS_BYTES, sAmount,
>-"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. This value can be incorrect because of race "
>-"conditions.");
>+  map<base::ProcessId, SharedBufferManagerParent*>::iterator it;
>+  for (it = SharedBufferManagerParent::sManagers.begin(); it != SharedBufferManagerParent::sManagers.end(); it++) {
>+    base::ProcessId pid = it->first;
>+    SharedBufferManagerParent *mgr = it->second;
>+
>+    std::map<int, android::sp<android::GraphicBuffer> >::iterator buf_it;
>+    for (buf_it = mgr->mBuffers.begin(); buf_it != mgr->mBuffers.end(); buf_it++) {
>+      nsresult rv;
>+      android::sp<android::GraphicBuffer> gb = buf_it->second;
>+      int bpp = android::bytesPerPixel(gb->getPixelFormat());
>+      int stride = gb->getStride();
>+      int height = gb->getHeight();
>+      int amount = bpp > 0 ?
>+        (stride * height * bpp) :
>+	// Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)
>+        (stride * height * 3 / 2);
>+
>+      nsPrintfCString gpath("gralloc/pid=%d/w=%d, h=%d, bpp=%d",
>+        pid, gb->getWidth(), height, bpp);
>+
>+// Due to MOZ_COLLECT_REPORT doesn't accept non-literal string as path,
>+// here we have 2 approaches to output report.
>+// Need feedback for pros and cons of each approach.
>+#if 1
>+// Approach 1:
>+//   Add a new macro which is similar to MOZ_COLLECT_REPORT, except it removes
>+//   NS_LITERAL_CSTRING wrapping from "path"
>+//
>+#define MOZ_COLLECT_CSTRING_REPORT(path, kind, units, amount, description)      \
>+  aHandleReport->Callback(EmptyCString(), path,                                 \
>+                          kind, units, amount,                                  \
>+                          NS_LITERAL_CSTRING(description), aData)
>+
>+      MOZ_COLLECT_CSTRING_REPORT(gpath, KIND_OTHER, UNITS_BYTES, amount,
>+          "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. This value can be incorrect because of race "
>+          "conditions.");
>+#else
>+// Approach 2:
>+// just call the callback of aHandleReport(which called by MOZ_COLLECT_REPORT originally),
>+// such that we can pass a nsPrintfCString directly.
>+//
>+      rv = aHandleReport->Callback(EmptyCString(), gpath, KIND_OTHER, UNITS_BYTES, amount, 
>+          NS_LITERAL_CSTRING(
>+            "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. This value can be incorrect because of race "
>+            "conditions."),
>+          aData);
>+#endif
>+        if ( rv != NS_OK) {
>+          return rv;
>+	}
>+    }
>   }
> 
>-  static int64_t sAmount;
>+  return NS_OK;
>+  }
> };
> 
> NS_IMPL_ISUPPORTS(GrallocReporter, nsIMemoryReporter)
> 
>-int64_t GrallocReporter::sAmount = 0;
>-
> void InitGralloc() {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   RegisterStrongMemoryReporter(new GrallocReporter());
> }
> 
>-map<base::ProcessId, SharedBufferManagerParent* > SharedBufferManagerParent::sManagers;
>-StaticAutoPtr<Monitor> SharedBufferManagerParent::sManagerMonitor;
>-int SharedBufferManagerParent::sBufferKey = 0;
>-
> SharedBufferManagerParent::SharedBufferManagerParent(Transport* aTransport, base::ProcessId aOwner, base::Thread* aThread)
>   : mTransport(aTransport)
>   , mThread(aThread)
> #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
>   , mBuffersMutex("BuffersMonitor")
> #endif
> {
>   if (!sManagerMonitor)
>     sManagerMonitor = new Monitor("Manager Monitor");
> 
>@@ -149,24 +189,20 @@ bool SharedBufferManagerParent::RecvAllocateGrallocBuffer(const IntSize& aSize,
> #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
>   sp<GraphicBuffer> outgoingBuffer = new GraphicBuffer(aSize.width, aSize.height, aFormat, aUsage);
> 
>   GrallocBufferRef ref;
>   ref.mOwner = mOwner;
>   ref.mKey = ++sBufferKey;
>   *aHandle = MagicGrallocBufferHandle(outgoingBuffer, ref);
> 
>   int bpp = 0;
>   bpp = android::bytesPerPixel(outgoingBuffer->getPixelFormat());
>-  if (bpp > 0)
>-    GrallocReporter::sAmount += outgoingBuffer->getStride() * outgoingBuffer->getHeight() * bpp;
>-  else // Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)
>-    GrallocReporter::sAmount += outgoingBuffer->getStride() * outgoingBuffer->getHeight() * 3 / 2;
> 
>   {
>     MutexAutoLock lock(mBuffersMutex);
>     mBuffers[sBufferKey] = outgoingBuffer;
>   }
> #endif
>   return true;
> }
> 
> bool SharedBufferManagerParent::RecvDropGrallocBuffer(const mozilla::layers::MaybeMagicGrallocBufferHandle& handle)
>@@ -174,24 +210,20 @@ bool SharedBufferManagerParent::RecvDropGrallocBuffer(const mozilla::layers::May
> #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
>   NS_ASSERTION(handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef, "We shouldn't interact with the real buffer!");
>   int bufferKey = handle.get_GrallocBufferRef().mKey;
>   sp<GraphicBuffer> buf = GetGraphicBuffer(bufferKey);
>   MutexAutoLock lock(mBuffersMutex);
>   NS_ASSERTION(mBuffers.count(bufferKey) == 1, "How can you drop others buffer");
>   mBuffers.erase(bufferKey);
> 
>   int bpp = 0;
>   bpp = android::bytesPerPixel(buf->getPixelFormat());
>-  if (bpp > 0)
>-    GrallocReporter::sAmount -= buf->getStride() * buf->getHeight() * bpp;
>-  else // Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)
>-    GrallocReporter::sAmount -= buf->getStride() * buf->getHeight() * 3 / 2;
> 
> #endif
>   return true;
> }
> 
> void SharedBufferManagerParent::DropGrallocBufferSync(SharedBufferManagerParent* mgr, mozilla::layers::SurfaceDescriptor aDesc)
> {
>   mgr->DropGrallocBufferImpl(aDesc);
> }
> 
>diff --git a/gfx/layers/ipc/SharedBufferManagerParent.h b/gfx/layers/ipc/SharedBufferManagerParent.h
>index 753292c..fc7a0a8 100644
>--- a/gfx/layers/ipc/SharedBufferManagerParent.h
>+++ b/gfx/layers/ipc/SharedBufferManagerParent.h
>@@ -25,20 +25,21 @@ class Thread;
> 
> namespace mozilla {
> #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> class Mutex;
> #endif
> 
> namespace layers {
> 
> class SharedBufferManagerParent : public PSharedBufferManagerParent
> {
>+friend class GrallocReporter;
> public:
>   /**
>    * Create a SharedBufferManagerParent for child process, and link to the child side before leaving
>    */
>   static PSharedBufferManagerParent* Create(Transport* aTransport, ProcessId aOtherProcess);
> 
>   /**
>    * Function for find the buffer owner, most buffer passing on IPC contains only owner/key pair.
>    * Use these function to access the real buffer.
>    */

Comment 4

[Solomon Chiu [:schiu]]

Please ignore comment#3, I made a mistake while editing the detail of attach.

Comment 5

[Solomon Chiu [:schiu]]

Comment on attachment 8419267 [details] [diff] [review]
WIP: Gralloc reporter

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +62,5 @@
> +        (stride * height * 3 / 2);
> +
> +      nsPrintfCString gpath("gralloc/pid=%d/w=%d, h=%d, bpp=%d",
> +        pid, gb->getWidth(), height, bpp);
> +

Let me recap the comment further, Here I propose 2 appoaches to output the report, due to MOZ_COLLECT_REPORT doesn't accept non-literal string as its second argument: "path". The first one is adding a new macro which is similar to MOZ_COLLECT_REPORT, except it removes NS_LITERAL_CSTRING wrapping from "path". And the second one is calling the callback of aHandleReport(which called by MOZ_COLLECT_REPORT originally), such that we can pass a nsPrintfCString directly. We need to know the pros and cos of each approach, or if there exist a better way to do the job.

Comment 6

[Solomon Chiu [:schiu]]

Attachment: Example memory-reports

Comment 7

[Solomon Chiu [:schiu]]

Attachment: Snapshot of example gralloc reporter

Comment 8

[Solomon Chiu [:schiu]]

Attachment: WIP: breaking down the memory report of gralloc

Comment 9

[Solomon Chiu [:schiu]]

Comment on attachment 8426370 [details] [diff] [review]
WIP: breaking down the memory report of gralloc

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +67,5 @@
> +// Due to MOZ_COLLECT_REPORT doesn't accept non-literal string as path,
> +// just call the callback of aHandleReport(which called by MOZ_COLLECT_REPORT originally),
> +// such that we can pass a nsPrintfCString directly.
> +//
> +      rv = aHandleReport->Callback(EmptyCString(), gpath, KIND_OTHER, UNITS_BYTES, amount,

Here we call aHandleReport->Callback() instead of using MOZ_COLLECT_REPORT, because the macro cannot accept a non-literal string as its second argument. An alternative way is create another macro similar to MOZ_COLLECT_REPORT, and accept non-literal strings as its second argument, which similar to the way used in http://dxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#2790.

Comment 10

[Solomon Chiu [:schiu]]

Hi Nicholas,

Could you please give me some advice about the way I breaking down the memory usage of gralloc in the WIP patch? Another thing also need your comment is: I use aHandleReport->Callback() instead of MOZ_COLLECT_REPORT, because the macro doesn't accept non-literal string as its second argument. Do you have any concern about my way?

Thanks.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8426370 [details] [diff] [review]
WIP: breaking down the memory report of gralloc

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

This looks good! Splitting up large about:memory buckets like this is often very useful.

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +44,5 @@
>    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
>                              nsISupports* aData)
>    {
> +  map<base::ProcessId, SharedBufferManagerParent*>::iterator it;
> +  for (it = SharedBufferManagerParent::sManagers.begin(); it != SharedBufferManagerParent::sManagers.end(); it++) {

The indentation here doesn't look right.

@@ +57,5 @@
> +      int stride = gb->getStride();
> +      int height = gb->getHeight();
> +      int amount = bpp > 0 ?
> +        (stride * height * bpp) :
> +	// Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)

"Special", and add a space after "formats", and add a period at the end, please.

@@ +61,5 @@
> +	// Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)
> +        (stride * height * 3 / 2);
> +
> +      nsPrintfCString gpath("gralloc/pid=%d/w=%d, h=%d, bpp=%d",
> +        pid, gb->getWidth(), height, bpp);

Use "width" and "height" instead of "w" and "h". Also, in memory reports we usually have some kind of identifying name with parameters in parentheses, so something like this would be good:

> "gralloc/pid(%d)/buffer(width=%d, height=%d, bpp=%d)"

There may be a better word to use than "buffer".

@@ +65,5 @@
> +        pid, gb->getWidth(), height, bpp);
> +
> +// Due to MOZ_COLLECT_REPORT doesn't accept non-literal string as path,
> +// just call the callback of aHandleReport(which called by MOZ_COLLECT_REPORT originally),
> +// such that we can pass a nsPrintfCString directly.

Not using the macro is fine, and there's no need for this comment.

@@ +83,4 @@
>    }
>  
> +  return NS_OK;
> +  }

The indentation looks wrong here, too.

Comment 12

[Nicholas Nethercote [inactive]]

BTW, you can copy+paste the text from about:memory. It's a lot easier than taking a screenshot :)

Comment 13

[Nicholas Nethercote [inactive]]

> How to use this memory tool is more important. As we talked during the
> meeting, please create bug for one click memory usage snapshot for memory
> analysis, like the following
> a. run the tool to capture current memory report
> b. reproduce something
> c. stop the tool to capture another new memory report
> d. display the difference between these two reports

I wasn't at the meeting, but I think we already have this covered: the tools/get_about_memory.py script lets you capture memory reports at arbitrary points in time, and then you can diff them in about:memory (in desktop Firefox) using the "Load and diff..." button.

Comment 14

[Solomon Chiu [:schiu]]

Attachment: Breaking down the memory report of gralloc

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8426882 [details] [diff] [review]
Breaking down the memory report of gralloc

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +19,5 @@
>  #include "ui/PixelFormat.h"
>  #endif
>  #include "nsThreadUtils.h"
>  
> +#include "nsPrintfCString.h"

Don't put extra line between #include unless you have specific reason and put it to previous line of nsThreadUtils.h in order to maintain alphabetic order.

@@ +44,4 @@
>    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
>                              nsISupports* aData)
>    {
> +  map<base::ProcessId, SharedBufferManagerParent*>::iterator it;

Many comments address by :njn seems not fix here. So I suggest fix the comment first or I'll send some duplicate comment.

Comment 16

[Nicholas Nethercote [inactive]]

And please ask me to review when the next version is put up. Thanks!

Comment 17

[Solomon Chiu [:schiu]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> And please ask me to review when the next version is put up. Thanks!

Hi Nicholas,

We need to do peer review first inside our team, and then ask formal review. I may still need to modify some code to make it better, and will ask for your review after modification.

Thanks.

Comment 18

[Solomon Chiu [:schiu]]

Attachment: Breaking down the memory report of gralloc

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8427654 [details] [diff] [review]
Breaking down the memory report of gralloc

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

Looks good, thanks!

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +32,5 @@
>  namespace layers {
>  
> +map<base::ProcessId, SharedBufferManagerParent* > SharedBufferManagerParent::sManagers;
> +StaticAutoPtr<Monitor> SharedBufferManagerParent::sManagerMonitor;
> +int SharedBufferManagerParent::sBufferKey = 0;

Does this need to be atomic? (BTW, I realize it was like this prior to your patch, but while we are here...)

@@ +57,5 @@
> +        int height = gb->getHeight();
> +        int amount = bpp > 0 ?
> +          (stride * height * bpp) :
> +          // Special case for BSP specific formats (mainly YUV formats, count it as normal YUV buffer).
> +          (stride * height * 3 / 2);

Nit: this is typically a better style for multi-line ternary expressions:

> value = cond
>       ? a
>       : b;

@@ +70,5 @@
> +              "resource, with much less available than generic RAM. When it's exhausted, "
> +              "graphics performance can suffer. This value can be incorrect because of race "
> +              "conditions."),
> +            aData);
> +        if ( rv != NS_OK) {

Nit: No space before |rv|.

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8427654 [details] [diff] [review]
Breaking down the memory report of gralloc

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

LGTM

Comment 21

[Solomon Chiu [:schiu]]

Attachment: Breaking down the memory report of gralloc

rebase to m+c, and carry r+

Comment 22

[Solomon Chiu [:schiu]]

try server result: https://tbpl.mozilla.org/?tree=Try&rev=1daa81fea74e

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/517a7e48664a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/517a7e48664a