915940 - Report graphics use of raw memory.

Status: NEW

(Core :: Graphics, defect)

Description

[Nick Cameron [:nrc]]

We should report our use of memory better. On a recent test I had 1.7Gb of memory used by gfx that about:memory knew nothing about.

It looks like we don't report on memory used by Moz2D AlignedArrays (http://dxr.mozilla.org/mozilla-central/source/gfx/2d/Tools.h#l113) or memory allocated for MemoryImage SurfaceDescriptors (http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ISurfaceAllocator.cpp#l87). Both of these uses can be HUGE. Not sure if shmem gets reported, but that would be large amounts of memory too.

Comment 1

[Hugh Nougher [:Hughman]]

So its known, on Windows 7+ there is the gpu-* reporters which give the values as seen by the graphics driver.

Having a better breakdown than gfx-* of what is contributing to the gpu-* values would definitely be useful, if possible.

Comment 2

[Nick Cameron [:nrc]]

(In reply to Hugh Nougher [:Hughman] from comment #1)
> So its known, on Windows 7+ there is the gpu-* reporters which give the
> values as seen by the graphics driver.
> 
> Having a better breakdown than gfx-* of what is contributing to the gpu-*
> values would definitely be useful, if possible.

I'm looking specifically at CPU memory here. But you are correct, a breakdown of GPU memory would be nice. Not sure if that is possible/easy. You should probably file another bug for that.

Comment 3

[Hugh Nougher [:Hughman]]

Ah. I just saw 'shmem' and thought about gpu-shared. And gpu-committed is how much of the processes address space its using. But I guess you are talking about another layer which I didn't think about like gfx system malloc memory.

Comment 4

[Nick Cameron [:nrc]]

Attachment: report on memory image memory use

Comment 5

[Nick Cameron [:nrc]]

Attachment: report on memory use in aligned arrays in moz2D

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 805066 [details] [diff] [review]
report on memory image memory use

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

Looks pretty good, but doesn't cover new-textures.

::: gfx/layers/ipc/ISurfaceAllocator.cpp
@@ +137,5 @@
>      case SurfaceDescriptor::TSurfaceDescriptorD3D10:
>        break;
>      case SurfaceDescriptor::TMemoryImage:
> +      GfxMemoryImageReporter::sAmount -= moz_malloc_usable_size((uint8_t*)aSurface->get_MemoryImage().data());
> +      delete [] (uint8_t*)aSurface->get_MemoryImage().data();

I'm pretty sure this can happen on both threads, I think we need to avoid races here.

Comment 7

[Nick Cameron [:nrc]]

Attachment: count new textures and use an atomic int for counting

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 805066 [details] [diff] [review]
report on memory image memory use

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

Did you read https://wiki.mozilla.org/Memory_Reporting before writing this?  I'm giving f+ because it's heading in the right direction but I'd like to see a revised version.

I'd be interested to see example output copy+pasted from about:memory for all three patches.

::: gfx/layers/ipc/ISurfaceAllocator.cpp
@@ +26,5 @@
>  
>  namespace mozilla {
>  namespace layers {
>  
> +int64_t GfxMemoryImageReporter::sAmount = 0;

As the wiki page describes, counter-based reporters are more error-prone than traversal-based reporters.  Could this be implemented via traversal instead?

@@ +89,5 @@
>      uint8_t *data = new (std::nothrow) uint8_t[stride * aSize.height];
>      if (!data) {
>        return false;
>      }
> +    GfxMemoryImageReporter::sAmount += moz_malloc_usable_size(data);

Using moz_malloc_usable_size() doesn't let DMD know that the memory has been measured.  You should use NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_{ALLOC,FREE}_FUN to generate functions that can be used here, that will integrate with DMD.

Or, if you convert it to a traversal-based reporter, you could use NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN instead.

::: gfx/layers/ipc/ISurfaceAllocator.h
@@ +139,5 @@
> +  friend class ISurfaceAllocator;
> +public:
> +  GfxMemoryImageReporter()
> +    : MemoryUniReporter("gfx-memory-image", KIND_HEAP, UNITS_BYTES,
> +                        "Memory shared between threads by texture clients and hosts.")

The path should have "explicit/" as a prefix, so that this is included in about:memory's "explicit" tree.  Currently it'll show up in the "other measurements" section.

"gfx-memory-image" seems much more general than the description.  Maybe "gfx-shared-textures" or something like that would be better?

Does this kind of memory occur on all platforms?  I don't recall seeing anything that looked like this when running DMD on Linux, though I may not have been looking at the right places.  Are there any good test cases I can try?

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 805067 [details] [diff] [review]
report on memory use in aligned arrays in moz2D

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

::: gfx/2d/Tools.h
@@ +101,5 @@
>  
>    MOZ_ALWAYS_INLINE ~AlignedArray()
>    {
> +    if (mStorage) {
> +      gAlignedArrayMemoryUsed -= sizeof(*mStorage);

Don't use |sizeof|;  use one of the MALLOC_SIZE_OF variants, again so that it integrates properly with DMD.

Again, this is a counter-based reporter, and it looks like it could be implemented as a traversal-based reporter.

::: gfx/thebes/gfxPlatform.cpp
@@ +322,5 @@
> +class GfxMoz2DMemoryReporter MOZ_FINAL : public MemoryUniReporter
> +{
> +public:
> +    GfxMoz2DMemoryReporter()
> +      : MemoryUniReporter("gfx-Moz2D-memory",

No caps in the path, please.

Again, you need an "explicit/" prefix.

We already have a "gfx" sub-tree in the "explicit" tree.  This reporter (and the one from the previous patch) should be put under it, i.e. change the path to "explicit/gfx/moz2d-memory".

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 805112 [details] [diff] [review]
count new textures and use an atomic int for counting

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

This needs to be folded in with the first patch, right?

A traversal-based counter would obviate the need for the atomic counter.

Comment 11

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> Comment on attachment 805112 [details] [diff] [review]
> count new textures and use an atomic int for counting
> 
> Review of attachment 805112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs to be folded in with the first patch, right?
> 
yes

Comment 12

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 805067 [details] [diff] [review]
> report on memory use in aligned arrays in moz2D
> 
> Review of attachment 805067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Tools.h
> @@ +101,5 @@
> >  
> >    MOZ_ALWAYS_INLINE ~AlignedArray()
> >    {
> > +    if (mStorage) {
> > +      gAlignedArrayMemoryUsed -= sizeof(*mStorage);
> 
> Don't use |sizeof|;  use one of the MALLOC_SIZE_OF variants, again so that
> it integrates properly with DMD.
> 

I think we do not have access to moz_malloc_size_of here. Moz2D is a standalone library and we only include mfbt, and avoid including other Mozilla headers. That is why I used sizeof here.

> Again, this is a counter-based reporter, and it looks like it could be
> implemented as a traversal-based reporter.
> 

For the same reason, I don't think I can write this as a traversal reporter.

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +322,5 @@
> > +class GfxMoz2DMemoryReporter MOZ_FINAL : public MemoryUniReporter
> > +{
> > +public:
> > +    GfxMoz2DMemoryReporter()
> > +      : MemoryUniReporter("gfx-Moz2D-memory",
> 
> No caps in the path, please.
> 

OK
> Again, you need an "explicit/" prefix.
> 
> We already have a "gfx" sub-tree in the "explicit" tree.  This reporter (and
> the one from the previous patch) should be put under it, i.e. change the
> path to "explicit/gfx/moz2d-memory".

However, most of the graphics reporters don't use explicit or explicit gfx, I presume that is an error? (See for e.g., http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.cpp?from=gfxASurface.cpp#l555 or the d2d VRAM memory reporters.). Should I still go for explicit/gfx?

Comment 13

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 805066 [details] [diff] [review]
> report on memory image memory use
> 
> Review of attachment 805066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you read https://wiki.mozilla.org/Memory_Reporting before writing this? 

Yes

> I'm giving f+ because it's heading in the right direction but I'd like to
> see a revised version.
> 
> I'd be interested to see example output copy+pasted from about:memory for
> all three patches.
> 

The third patch is really part of the first. The second is speculative - I have not actually got a test case where we report anything - I was wondering whether it was even worth landing to be honest, but I think it is likely to see more use as we use Moz2D rather than Thebes in more of our code. Dumps for the first patch coming up soon.

> ::: gfx/layers/ipc/ISurfaceAllocator.cpp
> @@ +26,5 @@
> >  
> >  namespace mozilla {
> >  namespace layers {
> >  
> > +int64_t GfxMemoryImageReporter::sAmount = 0;
> 
> As the wiki page describes, counter-based reporters are more error-prone
> than traversal-based reporters.  Could this be implemented via traversal
> instead?
> 

I considered it, but thought it would be more complicated - once allocated the memory is only referenced via a SurfaceDescriptor, which is a generated class (from IPDL). These are then passed around from thread to thread and not really kept tracked of - the lifetime management for them and their resources is an ongoing nightmare and is why we were leaking this memory image memory until Friday. So I am not sure how you would keep track of the SurfaceDescriptors to start the traversal or how you would avoid double counting or how/where you would add the SizeOf*This methods. Unless I am misunderstanding how the traversal reporters work, which is possible, if not likely :-)

> @@ +89,5 @@
> >      uint8_t *data = new (std::nothrow) uint8_t[stride * aSize.height];
> >      if (!data) {
> >        return false;
> >      }
> > +    GfxMemoryImageReporter::sAmount += moz_malloc_usable_size(data);
> 
> Using moz_malloc_usable_size() doesn't let DMD know that the memory has been
> measured.  You should use
> NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_{ALLOC,FREE}_FUN to generate functions
> that can be used here, that will integrate with DMD.
> 

I didn't see this in the wiki page. In fact it recommends moz_malloc_usable_size in preference to sizeof, which is why I used it here. I'll try with NS_MEMORY_REPORTER_...

> Or, if you convert it to a traversal-based reporter, you could use
> NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN instead.
> 
> ::: gfx/layers/ipc/ISurfaceAllocator.h
> @@ +139,5 @@
> > +  friend class ISurfaceAllocator;
> > +public:
> > +  GfxMemoryImageReporter()
> > +    : MemoryUniReporter("gfx-memory-image", KIND_HEAP, UNITS_BYTES,
> > +                        "Memory shared between threads by texture clients and hosts.")
> 
> The path should have "explicit/" as a prefix, so that this is included in
> about:memory's "explicit" tree.  Currently it'll show up in the "other
> measurements" section.
> 

See previous comments. "other" is where most of the current measurements are.

> "gfx-memory-image" seems much more general than the description.  Maybe
> "gfx-shared-textures" or something like that would be better?
> 

MemoryImage is the name of the classes and surface descriptor we are measuring. 'texture' is a little tricky here because it has connotations of GPU memory (even though we do 'texture' to mean more than GPU memory already) where we are measuring CPU memory. Perhaps gfx-heap-textures? (Although that could imply we include shmem, which this doesn't).

> Does this kind of memory occur on all platforms?  I don't recall seeing
> anything that looked like this when running DMD on Linux, though I may not
> have been looking at the right places.  Are there any good test cases I can
> try?

Potentially it can occur everywhere except b2g (which always uses shmem). It would only appear with OMTC so you would be unlikely to see it on Linux. You would see it on Android and Mac currently. Windows soon, Linux one day. You could try turning omtc on for Linux (you would need to support opengl and force on HWA - layers.acceleration.force-enabled) - layers.offmainthreadcomposition.enabled. You should then see memory images getting used. For really big use, try an extra large canvas.

Comment 14

[Nicholas Nethercote [inactive]]

> > ::: gfx/2d/Tools.h
> > @@ +101,5 @@
> > >  
> > >    MOZ_ALWAYS_INLINE ~AlignedArray()
> > >    {
> > > +    if (mStorage) {
> > > +      gAlignedArrayMemoryUsed -= sizeof(*mStorage);
> > 
> > Don't use |sizeof|;  use one of the MALLOC_SIZE_OF variants, again so that
> > it integrates properly with DMD.
> 
> I think we do not have access to moz_malloc_size_of here. Moz2D is a
> standalone library and we only include mfbt, and avoid including other
> Mozilla headers. That is why I used sizeof here.

Does it not even have mozalloc.h?  I thought even MFBT had access to that.

This is a problem.  DMD integration is really important;  without it, the numbers in about:memory could be completely wrong and we'd have no way of knowing.  If Moz2D does have access to mozalloc.h, we might be able to get this working (DMD integration doesn't require much else).

Failing that, it's common for libraries to allow you to specify the malloc/free functions -- you just pass in function pointers.  If Moz2D provides that, that will suffice -- we use this for several 3rd party libraries such as ICU and Freetype.  If Moz2D doesn't provide that, perhaps it should :)

> > Again, you need an "explicit/" prefix.
> > 
> However, most of the graphics reporters don't use explicit or explicit gfx,
> I presume that is an error? (See for e.g.,
> http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.
> cpp?from=gfxASurface.cpp#l555 or the d2d VRAM memory reporters.). Should I
> still go for explicit/gfx?

The "explicit" tree holds memory allocations done with malloc/new and with mmap/VirtualAlloc.  AIUI, the gfx reporters measure memory that doesn't fit that pattern.

> I considered it, but thought it would be more complicated - once allocated
> the memory is only referenced via a SurfaceDescriptor, which is a generated
> class (from IPDL). These are then passed around from thread to thread and
> not really kept tracked of - the lifetime management for them and their
> resources is an ongoing nightmare and is why we were leaking this memory
> image memory until Friday. So I am not sure how you would keep track of the
> SurfaceDescriptors to start the traversal or how you would avoid double
> counting or how/where you would add the SizeOf*This methods. Unless I am
> misunderstanding how the traversal reporters work, which is possible, if not
> likely :-)

How do you free that memory?  The code for a traversal-based reporter often looks similar to the code that frees a data structure.

> Perhaps gfx-heap-textures?

Seems reasonable.  Certainly better than "gfx-memory-image".

Comment 15

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> > > ::: gfx/2d/Tools.h
> > > @@ +101,5 @@
> > > >  
> > > >    MOZ_ALWAYS_INLINE ~AlignedArray()
> > > >    {
> > > > +    if (mStorage) {
> > > > +      gAlignedArrayMemoryUsed -= sizeof(*mStorage);
> > > 
> > > Don't use |sizeof|;  use one of the MALLOC_SIZE_OF variants, again so that
> > > it integrates properly with DMD.
> > 
> > I think we do not have access to moz_malloc_size_of here. Moz2D is a
> > standalone library and we only include mfbt, and avoid including other
> > Mozilla headers. That is why I used sizeof here.
> 
> Does it not even have mozalloc.h?  I thought even MFBT had access to that.
> 
> This is a problem.  DMD integration is really important;  without it, the
> numbers in about:memory could be completely wrong and we'd have no way of
> knowing.  If Moz2D does have access to mozalloc.h, we might be able to get
> this working (DMD integration doesn't require much else).
> 

OK, I'll investigate that.

> Failing that, it's common for libraries to allow you to specify the
> malloc/free functions -- you just pass in function pointers.  If Moz2D
> provides that, that will suffice -- we use this for several 3rd party
> libraries such as ICU and Freetype.  If Moz2D doesn't provide that, perhaps
> it should :)

And this if that doesn't work. I don't think there is a facility to do that, but I might be wrong. Bas might be able to weigh in on this and whether we could add it if not.
> 
> > > Again, you need an "explicit/" prefix.
> > > 
> > However, most of the graphics reporters don't use explicit or explicit gfx,
> > I presume that is an error? (See for e.g.,
> > http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.
> > cpp?from=gfxASurface.cpp#l555 or the d2d VRAM memory reporters.). Should I
> > still go for explicit/gfx?
> 
> The "explicit" tree holds memory allocations done with malloc/new and with
> mmap/VirtualAlloc.  AIUI, the gfx reporters measure memory that doesn't fit
> that pattern.
> 

I chatted a little bit about this with Hughman on irc. I think some of them do and it is a bug that they are not in the explicit tree, but that is out of scope for this bug. I agree that these reporters belong under explicit.

> > I considered it, but thought it would be more complicated - once allocated
> > the memory is only referenced via a SurfaceDescriptor, which is a generated
> > class (from IPDL). These are then passed around from thread to thread and
> > not really kept tracked of - the lifetime management for them and their
> > resources is an ongoing nightmare and is why we were leaking this memory
> > image memory until Friday. So I am not sure how you would keep track of the
> > SurfaceDescriptors to start the traversal or how you would avoid double
> > counting or how/where you would add the SizeOf*This methods. Unless I am
> > misunderstanding how the traversal reporters work, which is possible, if not
> > likely :-)
> 
> How do you free that memory?  The code for a traversal-based reporter often
> looks similar to the code that frees a data structure.
> 

You call this http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ISurfaceAllocator.cpp?from=ISurfaceAllocator.cpp#l114 to free the memory. Who calls it and when is a bit complicated. We would have to traverse the layer trees on both the compositor and main threads to find the 'texture' objects which currently hold surface descriptors of this type. It would be a lot of work. And since the surface descriptors can be passed with move semantics between threads we would have to be able to pause the world and guarantee that inter-thread communication has finished when we do it (maybe we do this already when we measure, in which case things would be a little easier).

The whole mechanism is ugly and sad, frankly. I just fixed a massive leak when we failed to free some of this memory (that gave me the motivation to write this reporter), so basing the reporter on how we free the memory seems like it could potentially miss some leaks.

> > Perhaps gfx-heap-textures?
> 
> Seems reasonable.  Certainly better than "gfx-memory-image".

Comment 16

[Nicholas Nethercote [inactive]]

> The whole mechanism is ugly and sad, frankly. I just fixed a massive leak
> when we failed to free some of this memory (that gave me the motivation to
> write this reporter), so basing the reporter on how we free the memory seems
> like it could potentially miss some leaks.

Fair enough.  BTW, my prejudice against counter-based reporters is based on real experience -- we've had numerous bugs in the past, usually identified when the result goes negative, because the increment/decrement places don't match.  (That's why the docs are more thorough in describing traversal-based reporters :)  But counter-based is sometimes necessary, though usually it's because of hard code constraints (such as the use of 3rd party libs) rather than code complexity!

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 805112 [details] [diff] [review]
count new textures and use an atomic int for counting

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

::: gfx/layers/client/TextureClient.cpp
@@ +212,5 @@
>    MOZ_ASSERT(!mBuffer);
>    mBuffer = new uint8_t[aSize];
> +  if (mBuffer) {
> +    GfxMemoryImageReporter::sAmount += moz_malloc_usable_size(mBuffer);
> +  }

new is infallible, no need to check it.

Comment 18

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 805066 [details] [diff] [review]
> report on memory image memory use
> 
> Review of attachment 805066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd be interested to see example output copy+pasted from about:memory for
> all three patches.
> 

For the first patch (with modifications for reviewers), pre bug 915944 and after refreshing a page with a large canvas on it, OMTC w/ d3d11:

842.15 MB (100.0%) -- explicit
├──763.06 MB (90.61%) -- gfx
│  ├──762.94 MB (90.59%) ── heap-textures
│  └────0.12 MB (00.01%) ++ (5 tiny)

Same, but post bug 915944:

├──0.06 MB (00.09%) -- gfx
│   │  ├──0.03 MB (00.04%) ── font-list
│   │  ├──0.02 MB (00.03%) ── font-cache
│   │  ├──0.01 MB (00.01%) ── font-shaped-words
│   │  ├──0.01 MB (00.01%) ── font-charmaps
│   │  └──0.00 MB (00.00%) ── heap-textures

Comment 19

[Nick Cameron [:nrc]]

Attachment: report on memory image memory use

Combined patches one and three + reviewer requested changes.

Comment 20

[Nick Cameron [:nrc]]

Attachment: fix a possible bug in the reporting of gfxWindowsSurface

Comment 21

[Nick Cameron [:nrc]]

Attachment: AlignedArray (Moz2D) memory reporter

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 806449 [details] [diff] [review]
report on memory image memory use

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

Looks good.  Thanks!

::: gfx/layers/client/TextureClient.cpp
@@ +210,5 @@
>  MemoryTextureClient::Allocate(uint32_t aSize)
>  {
>    MOZ_ASSERT(!mBuffer);
>    mBuffer = new uint8_t[aSize];
> +  GfxMemoryImageReporter::DidAlloc(mBuffer);

Re the |DidAlloc| and |WillFree| naming -- I understand you're indicating the time relationship between the reporting and the alloc/free, but |OnAlloc| and |OnFree| (or variants thereof) are the standard names used in several other memory reporters.  So I think they should be used here too.

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 807007 [details] [diff] [review]
AlignedArray (Moz2D) memory reporter

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

This is not the right patch -- you've accidentally uploaded the 2nd patch again.

Comment 24

[Nick Cameron [:nrc]]

Attachment: AlignedArray (Moz2D) memory reportermem2.patch

Ah, sorry about that.

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 807016 [details] [diff] [review]
AlignedArray (Moz2D) memory reportermem2.patch

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

Looks good, except for the lack of DMD integration due to the use of vanilla moz_malloc_size_of().  I'll try to integrate DMD myself with these patches.  Back soon.

::: gfx/2d/Factory.cpp
@@ +504,5 @@
>  
> +int64_t gAlignedArrayMemoryUsed = 0;
> +
> +int64_t
> +Factory::GetArrayMemoryUsage()

Nit: GetAlignedArrayMemoryUsed() would fit the other names better.

::: gfx/2d/Tools.h
@@ +105,1 @@
>      delete [] mStorage;

The reason counter-based reporters are error-prone is that it's easy to fail to update the counter in all the places it's needed.  Especially if the code is changed later on by somebody who doesn't understand it well.

If this function called Dealloc(), you wouldn't need to update the counter, and there would be one less place to get it wrong.

@@ +118,3 @@
>      delete [] mStorage;
>      mStorage = new (std::nothrow) T[aSize + (alignment - 1)];
> +    gAlignedArrayMemoryUsed += moz_malloc_size_of(mStorage);

An aside... libc's realloc() has the following property:  If realloc() fails the original block is left untouched; it is  not  freed or moved.  This function does not have that property -- the delete[] will always succeed but the new[] might fail, leaving you with nothing.  So I think a name other than "Realloc" should be used ("DeallocThenAlloc")?

BTW, it's a bit odd having Realloc() and Dealloc() but no Alloc().  No matter, I guess.

Comment 26

[Nicholas Nethercote [inactive]]

> > +    gAlignedArrayMemoryUsed += moz_malloc_size_of(mStorage);

And I assume accesses such as these are single-threaded, and so don't need Atomic<> ?

Comment 27

[Nicholas Nethercote [inactive]]

Oh, and I suggest changing "gfx/moz2d-memory" to "gfx/moz2d".  I generally avoid putting "memory" in the paths because it doesn't add anything.

Comment 28

[Nicholas Nethercote [inactive]]

A final nit:  in the first patch, |GfxMemoryImageReporter| should be renamed |GfxHeapTexturesReporter|, so the class name matches the "gfx/heap-textures" path.  Thanks.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: Integrate with DMD.

I just included nsIMemoryReporter.h and it works fine.  I think this works,
despite moz2d being built standalone, because the changes I made can all be
obtained from the header file -- it doesn't need to link with any .o code.

I tried to test (on Linux) by enabling layers.acceleration.force-enabled and
layers.offmainthreadcomposition.enabled, and although I got non-zero
gfx/heap-textures, I still got zero gfx/moz2d-memory, so the DMD integration
isn't actually tested.  But it's simple enough that I'd be surprised if it
didn't work.

Assuming you're happy with this patch, I guess just fold it into the previous
one before landing.

Comment 30

[Nick Cameron [:nrc]]

Comment on attachment 807070 [details] [diff] [review]
Integrate with DMD.

Adding Bas for review as to whether its OK, to pull in nsIMemoryReporter.

Comment 31

[Nick Cameron [:nrc]]

Comment on attachment 807070 [details] [diff] [review]
Integrate with DMD.

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

lgtm, but I think there will be issues with pulling in the nsIMemoryReporter header - we would need a copy in the Moz2D repo, unless we can grab it the same way we do with mfbt. That duplication sounds bad to me. Anyway, I'll leave it up to Bas to decide.

Comment 32

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #27)
> Oh, and I suggest changing "gfx/moz2d-memory" to "gfx/moz2d".  I generally
> avoid putting "memory" in the paths because it doesn't add anything.

gfx-moz2d-heap? Because there is other memory used by moz2d in about:memory.

Comment 33

[Nicholas Nethercote [inactive]]

> gfx-moz2d-heap? Because there is other memory used by moz2d in about:memory.

"heap" is almost as vague as "memory".

The report description says "CPU memory used by Moz2D."  So "gfx/moz2d-cpu" would match that.  But maybe that description is too vague as well?  What's the other moz2d memory that's measure, and how does this differ?  "gfx/moz2d-aligned-arrays" matches what the code is doing.

Comment 34

[Nick Cameron [:nrc]]

(In reply to Nicholas Nethercote [:njn] from comment #33)
> > gfx-moz2d-heap? Because there is other memory used by moz2d in about:memory.
> 
> "heap" is almost as vague as "memory".
> 
> The report description says "CPU memory used by Moz2D."  So "gfx/moz2d-cpu"
> would match that.  But maybe that description is too vague as well?  What's
> the other moz2d memory that's measure, and how does this differ? 
> "gfx/moz2d-aligned-arrays" matches what the code is doing.

The other reporters are all for GPU memory, so cpu or heap is good to distinguish this as main memory vs video memory. (If we use other memory in Moz2D I would envisage adding it to this reporter rather than creating new reporters, which is why I didn't go with -aligned-arrays).

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 806452 [details] [diff] [review]
fix a possible bug in the reporting of gfxWindowsSurface

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

::: gfx/thebes/gfxWindowsSurface.cpp
@@ +63,4 @@
>      if (CairoStatus() == 0)
>          mDC = cairo_win32_surface_get_dc(CairoSurface());
> +    else {
> +        RecordMemoryUsed(size.width * size.height * 4 + sizeof(gfxWindowsSurface));

This should be in the success branch not the failure branch. While you're at it you should change CairoStatus() == 0 to CairoStatus() == CAIRO_STATUS_SUCCESS

Comment 36

[Nick Cameron [:nrc]]

Attachment: fix a possible bug in the reporting of gfxWindowsSurface

Whoops, sorry about that. With the correct fix this time.

Comment 37

[Bas Schouten (:bas.schouten)]

Comment on attachment 807016 [details] [diff] [review]
AlignedArray (Moz2D) memory reportermem2.patch

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

::: gfx/2d/Tools.h
@@ +100,5 @@
>    }
>  
>    MOZ_ALWAYS_INLINE ~AlignedArray()
>    {
> +    gAlignedArrayMemoryUsed -= moz_malloc_size_of(mStorage);

We can't use this in Moz2D as the implementation lives in mozalloc.cpp which is not included.

Comment 38

[Bas Schouten (:bas.schouten)]

Comment on attachment 807070 [details] [diff] [review]
Integrate with DMD.

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

::: gfx/2d/Tools.h
@@ +5,5 @@
>  
>  #ifndef MOZILLA_GFX_TOOLS_H_
>  #define MOZILLA_GFX_TOOLS_H_
>  
> +#include "nsIMemoryReporter.h"

Can't include this in Moz2D.

Comment 39

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/505fe9841b16

Just the first patch.

Comment 40

[Nicholas Nethercote [inactive]]

> > +    gAlignedArrayMemoryUsed -= moz_malloc_size_of(mStorage);
> 
> We can't use this in Moz2D as the implementation lives in mozalloc.cpp which
> is not included.

It builds fine for me on Linux, but maybe I just got lucky.

The only way forward that I can see is to modify Moz2D so it can be configured to take a custom allocator.

Comment 41

[Ed Morley [:emorley]]

As bizarre as it seems, this appears to have been the cause of Windows-only, PGO-only compilation failures (both failed and working builds were clobbers, and there are several retriggers), so without anything else to go on, had to back this out, sorry:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b287f2bd6c7e

Regression range:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=winnt.*pgo&tochange=f74b1fe6bcc8&fromchange=44cba3ea6b9d

I'll trigger PGO on tip and we should know for sure in a few hours.

Comment 42

[Ed Morley [:emorley]]

The retrigger after the backout has now been running 50mins longer the the failed runs did, so looking like it's confirmed:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=winnt.*pgo&rev=b287f2bd6c7e

Comment 43

[Nick Cameron [:nrc]]

The gfxWindowsSurface patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/072594314b13

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/072594314b13

Comment 45

[Nicholas Nethercote [inactive]]

Comment on attachment 807016 [details] [diff] [review]
AlignedArray (Moz2D) memory reportermem2.patch

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

Oh, I never r+'d this.  Since bas r-'d it, I guess it doesn't matter.

Comment 46

[Nick Cameron [:nrc]]

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

Pushing the first patch again. I've been assured by Ehsan that the PGO bustage was known and fixed and that this time it would work. Don't let me down Ehsan!

Comment 47

[(no longer active)]

Oh, I'm so sorry Nick.  It seems like when I said "this regression has been dealt with", what I meant to say was "by backing out your patch." :(

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Nick Cameron [:nrc] from comment #46)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ac77b26ec1ee
> 
> Pushing the first patch again. I've been assured by Ehsan that the PGO
> bustage was known and fixed and that this time it would work. Don't let me
> down Ehsan!

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9f480c4e89c1

Comment 51

[Nick Cameron [:nrc]]

Green on Try with PGO now: https://tbpl.mozilla.org/?tree=Try&rev=c2ee4e6351ad

I'll land this once inbound opens.

Comment 52

[(no longer active)]

(In reply to comment #51)
> Green on Try with PGO now: https://tbpl.mozilla.org/?tree=Try&rev=c2ee4e6351ad

\o/

Comment 53

[Nick Cameron [:nrc]]

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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d90adcd9929f
https://hg.mozilla.org/mozilla-central/rev/77302975dfd2

Comment 55

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 56

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 58

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.