921474 - Add a memory reporter for Memory and Shmem texture clients

Status: RESOLVED DUPLICATE of bug 915940

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

This will remove some of the unclassified memory (in the case of MemorytextureClient) and give us a better idea of how much memory we use to share stuff with the compositor.
Gralloc already has a memory reporter.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Memory reporter for ShmemTextureClient and MemoryTextureClient

I implemented this reporter following the steps of the already implemented gralloc one. It also has the same apparent "quirks", not sure it is a big deal:
 - it is created once and never deleted (we could delete it when shutting down firefox)
 - it is not thread-safe: both the ImageBridge thread and the main thread use it, so Add() and Remove() could race. Since it is just a reporting tool and it doesn't affect Gecko's behavior, I suppose it is best to have it not thread safe and risk to rarely give wrong data, rather than slowing Gecko down with a mutex when allocating/deallocating texture data.

If either of the above two items are not acceptable, then we should also fix the gralloc reporter.

Also, worth mentionning: this implementation cheats a little bit because the shared memory is always reported on the client side, even though sometimes it may be deallocated on the host side. So when the client sends to the host the order to deallocate memory, it also reports the memory as being deallocated, even though it will be deallocated a bit later.
If we screw something up between the moment when we tear-down the texture client and the moment when the host deallocates the data, the potential leak will not show in about:memory.

Comment 2

[Nicolas Silva [:nical]]

Also, this is only for the new textures.

Comment 3

[Nicolas Silva [:nical]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=00be9bd8a957

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 811127 [details] [diff] [review]
Memory reporter for ShmemTextureClient and MemoryTextureClient

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

I'm not convinced that the patch is correct and handles memory being passed around. Some better code comments would convince me.

::: gfx/layers/client/CompositableClient.cpp
@@ +237,4 @@
>        mTexturesToRemoveCallbacks[aClient->GetID()] = data;
> +    } else {
> +      data->ForgetSharedData();
> +      delete data;

Looks like this is a behavior change and doesn't belong in this bug. Where was the previous delete?

::: gfx/layers/client/TextureClient.cpp
@@ +128,2 @@
>    {
> +    GetMemoryTextureReporter()->Remove(mBufferSize);

Previously it was safe to call DeallocateSharedData when mBuffer was null but that's no longer the case. You should MOZ_ASSERT(mBuffer) here.

@@ +130,4 @@
>      delete[] mBuffer;
>    }
>  
> +  virtual void ForgetSharedData() MOZ_OVERRIDE

When is this ForgetSharedData used over Deallocate? When the data is passed to another actor? I don't see anything counting the buffer for the other side. Can we document near the texture memory reporter how memory is counted when it's passed over IPDL to a remote side.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Benoit Girard (:BenWa) from comment #4)
> Comment on attachment 811127 [details] [diff] [review]
> Memory reporter for ShmemTextureClient and MemoryTextureClient
> 
> Review of attachment 811127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not convinced that the patch is correct and handles memory being passed
> around. Some better code comments would convince me.

challenge accepted

> 
> ::: gfx/layers/client/CompositableClient.cpp
> @@ +237,4 @@
> >        mTexturesToRemoveCallbacks[aClient->GetID()] = data;
> > +    } else {
> > +      data->ForgetSharedData();
> > +      delete data;
> 
> Looks like this is a behavior change and doesn't belong in this bug. Where
> was the previous delete?

I reorganized things a bit here because it helped regroup the GetFooReporter()->Remove(size) calls to avoid putting them in too many different places, getting mixed up and forgetting to report things (or doing it twice).

Previously the data was only created if (!(aClient->GetFlags() & TEXTURE_DEALLOCATE_HOST)) in which case the data is stored in a map and deleted later (see CompositableClient::OnReplyTextureRemoved)

I can put this change in a separate patch if you want, but is so small I didn't feel the need to do that.

> 
> @@ +130,4 @@
> >      delete[] mBuffer;
> >    }
> >  
> > +  virtual void ForgetSharedData() MOZ_OVERRIDE
> 
> When is this ForgetSharedData used over Deallocate? When the data is passed
> to another actor?

It is used when the client tells the host to deallocate the shared memory. In this case the client must not deallocate the data and must not keep references to it since te host will deallocate it soon.

> I don't see anything counting the buffer for the other
> side. Can we document near the texture memory reporter how memory is counted
> when it's passed over IPDL to a remote side.

New textures don't transfer ownership of the shared data. They do actually share it, so if you want to count the memory on the host side, you will count the same memory twice. Counting on the client side is best because for B2G you get to to count memory allocation per app.

Comment 6

[Benoit Girard (:BenWa)]

That sounds good. I'm convinced. With your last point commented at the relevant place(s) in the patch I'm happy.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Memory reporter for ShmemTextureClient and MemoryTextureClient

This version is more documented and adds a few asserts as you asked.

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 811221 [details] [diff] [review]
Memory reporter for ShmemTextureClient and MemoryTextureClient

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

Much better!

::: gfx/layers/client/TextureClient.cpp
@@ +326,5 @@
>    MOZ_COUNT_DTOR(MemoryTextureClient);
>    if (ShouldDeallocateInDestructor()) {
>      // if the buffer has never been shared we must deallocate it or ir would
>      // leak.
> +    GetMemoryTextureReporter()->Remove(mBufSize);

MOZ_ASSERT(mBuffer)

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +72,5 @@
>    }
>  
> +  virtual void ForgetSharedData() MOZ_OVERRIDE
> +  {
> +    mGrallocActor = nullptr;

Why are we not counting gralloc buffers?

Comment 9

[Nicolas Silva [:nical]]

We already count gralloc buffers, see ShadowLayerUtilsGralloc.

Comment 10

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a77f7d610829

Comment 11

[Ryan VanderMeulen [:RyanVM]]

1.) This landed with the wrong bug # in the commit message.
2.) It leaked like crazy.

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2f60923d73

https://tbpl.mozilla.org/php/getParsedLog.php?id=28480233&tree=Mozilla-Inbound

Comment 12

[Nick Cameron [:nrc]]

I think this is a dup of bug 915940. Isn't shmem already reported? So the only thing you need to report is MemoryImages which are done in 915940 (backed out for PGO failure of all things, but on its way back at some point). Also, all memory reporters should get a review from njn.

Comment 13

[Nick Cameron [:nrc]]

I don't mean to bitch too much (and this is a general comment, not directed at nical), but this is one reason I dislike the idiom of filing a bug, assigning it, and upload a patch all at once. If you are thinking of working on something, file a bug, then assign it when you are going to start work, and upload the patch when you are done - then if someone else is working on the same thing there is an opportunity to notice.

Comment 14

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #12)
> I think this is a dup of bug 915940. Isn't shmem already reported? So the
> only thing you need to report is MemoryImages which are done in 915940
> (backed out for PGO failure of all things, but on its way back at some
> point).

My apologies, I didn't notice that bug and I needed to get this reporting as I was working on memory consumption.
I think it is still useful to track Shmem texture clients because we use shmem for a lot of different things, so knowing what portion of it is dedicated to sharing textures between the content and compositor processes is worthwhile.

> Also, all memory reporters should get a review from njn.

Good to know, I'll make sure to get his reviews next time.