689870 - Add memory reporter for (shared) GPU memory

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Hugh Nougher [:Hughman]]

A memory reporter or such to show the amount of memory used by GPU for Windows Vista/7 would be very helpful. The current gfx-* section seems to be something else.

As to how to get this information, I have no idea. The only program I know which shows GPU memory usage per application is Process Explorer, which I suggest is used in the mean time for working out if GPU shared memory is a factor in some reports of excessive memory usage.

As for reasons to have it showing in about:memory there are a few:
- GPU shared memory is counted in a application's private bytes (at least for win vista/7).
- Nightly with a new profile showing just about:home uses 18.6MB Dedicated GPU and 3.4MB Shared GPU memory on my windows 7 machine.
- In my normal Aurora session it uses 112MB Dedicated and 43MB shared on average.
- Systems which do not have any dedicated GPU memory would store all shared memory (which I believe is the cause in bug 629606).
- bug 678859 comment 8 and 12 is a prime example where this value in about:memory would be very helpful.

Comment 1

[(no longer active)]

Does any of you Windows folks know what API can be used to get this information?

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I don't know of any.  It's apparently possible to get system-wide GPU memory information on Vista+ (http://msdn.microsoft.com/en-us/windows/hardware/gg487348.aspx) but that's not what we want here.

Monitoring Process Explorer with Process Monitor reveals that it loads a bunch of NVidia DLLs (on my machine).  Maybe there are some video-card-manufacturer-specific APIs that can be used to get this information?

Comment 3

[André Ziegler]

I posted the structure in the Sysinternals forum:

http://forum.sysinternals.com/how-process-explorer-get-gpu-usage_topic26875_post131428.html#131428

it is part of the Win8 WDK. wj32 already implemented it in his Process Hacker.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

http://processhacker.svn.sourceforge.net/viewvc/processhacker/2.x/trunk/plugins/ExtendedTools/gpumon.c?revision=4770&view=markup&pathrev=4770

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

See bug 638549 for how the equivalent was implemented for WebGL.

Comment 6

[Hugh Nougher [:Hughman]]

I am going to have a go at this though I may have strange questions.

I looked over the WebGL equivalent. I think I understood how to make the memory reporters work so I made a sort of draft class which implemented two reporters called d3d/dedicated-memory and d3d/shared-memory (I'll get better names for these later when something works).

I also drafted a class called GPUMemoryReporter which I am going to not use as a singleton as WebGLMemoryReporter was where each memory reporter instance contains an adapter LUID (I think that is the grouping structure in the comment 4 code).

Now I come to problems that I have almost no idea how to solve. Most importantly, where am I supposed to put this class? I would guess somewhere where the adapter LUIDs already exist or the d3d10 area?

Also I think it will need d3dkmt.h so what is the correct method of getting that?
Then there is some gdi32.dll stuff which I am wondering about...

Hope I am on the right track so far!

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Hugh Nougher [:Hughman] from comment #6)
> I am going to have a go at this though I may have strange questions.
> 
> I looked over the WebGL equivalent. I think I understood how to make the
> memory reporters work so I made a sort of draft class which implemented two
> reporters called d3d/dedicated-memory and d3d/shared-memory (I'll get better
> names for these later when something works).
> 
> I also drafted a class called GPUMemoryReporter which I am going to not use
> as a singleton as WebGLMemoryReporter was where each memory reporter
> instance contains an adapter LUID (I think that is the grouping structure in
> the comment 4 code).

I think you should be able to pull this from the D3D device that we keep around.


> Now I come to problems that I have almost no idea how to solve. Most
> importantly, where am I supposed to put this class? I would guess somewhere
> where the adapter LUIDs already exist or the d3d10 area?

The right place is probably gfx/thebes/gfxWindowPlatform.cpp

> 
> Also I think it will need d3dkmt.h so what is the correct method of getting
> that?

Is d3dkmt.h from the Windows DDK? If so it might be easiest to create our own header that duplicates the needed definitions.

> Then there is some gdi32.dll stuff which I am wondering about...

Please ask.

> Hope I am on the right track so far!

Post your code and I'll try to give more concrete feedback.

Comment 8

[Hugh Nougher [:Hughman]]

Attachment: WIP-V0.1

So here is the changes I have made so far. Hope I got the method of making a patch correct.

In this it registers 4 multi-reporters which constantly add 2B each to dedicated and 3B to shared d3d in about:memory.

It also dynamically links up 3 functions from gdi32.dll using d3dkmt.h. This part gave me lots of trouble with NTSTATUS not being defined by windows.h and needing the function definitions compiled like C.

Next would be to get setupapi.dll linked in then starting collecting adapters and actual memory usage.

I am also considering moving the class definition out of the .h into the .cpp because I dont think it will need to be used anywhere else. Is this a good idea?

Time to find out if Im on the right track :|

Comment 9

[Justin Lebar (not reading bugmail)]

> I am also considering moving the class definition out of the .h into the .cpp because I 
> dont think it will need to be used anywhere else. Is this a good idea?

Yes, in general.  You can also put the class in an anonymous namespace (namespace { class Foo {}; } ), which helps the linker by telling it that no other file will use that class.

Where did this d3dkmt.h file come from?  It looks like it was copied from somewhere?

I have no idea bout the D3D stuff, but the memory reporters look like they're on the right track.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Hugh Nougher [:Hughman] from comment #8)
> Created attachment 573160 [details] [diff] [review] [diff] [details] [review]
> WIP-V0.1
> 
> So here is the changes I have made so far. Hope I got the method of making a
> patch correct.
> 
> In this it registers 4 multi-reporters which constantly add 2B each to
> dedicated and 3B to shared d3d in about:memory.
> 
> It also dynamically links up 3 functions from gdi32.dll using d3dkmt.h. This
> part gave me lots of trouble with NTSTATUS not being defined by windows.h
> and needing the function definitions compiled like C.
> 
> Next would be to get setupapi.dll linked in then starting collecting
> adapters and actual memory usage.
> 
> I am also considering moving the class definition out of the .h into the
> .cpp because I dont think it will need to be used anywhere else. Is this a
> good idea?
> 
> Time to find out if Im on the right track :|

You should be able to get AdapterLUID the following way:
DXGI_ADAPTER_DESC contains AdapterLUID
You can get a DXGI_ADAPTER_DESC from IDXGIAdapter::GetDesc();

You can get a IDXGIAdapter from IDXGIDevice::GetAdapater()

and you should be able to get a IDXGIDevice from our D3DDevice10 device using QueryInterface()

This is probably a better way to get the AdapterLUID than by using setupapi.dll

Comment 11

[Hugh Nougher [:Hughman]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Where did this d3dkmt.h file come from?  It looks like it was copied from
> somewhere?

I copied it from http://processhacker.svn.sourceforge.net/viewvc/processhacker/2.x/trunk/plugins/ExtendedTools/d3dkmt.h?revision=4758&view=markup and made some tiny changes to it so the function definitions would compile like in C.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> You can get a IDXGIAdapter from IDXGIDevice::GetAdapater()

There is one thing I keep wondering about... what happens when there is two adapters? Would that mean that there needs to be two devices if firefox had a window on each? Then what about sli/crossfire setups?

Another is what happens when the adapter or driver is reset. I have seems cases where due to a bug in the driver that win7 would kill the old driver and start again causing some programs to loose visual though firefox is usually ok. Updating drivers also causes this. When this happens does the adapter stay the same?

Comment 12

[Hugh Nougher [:Hughman]]

(In reply to Hugh Nougher [:Hughman] from comment #11)
> (In reply to Justin Lebar [:jlebar] from comment #9)
> > Where did this d3dkmt.h file come from?  It looks like it was copied from
> > somewhere?
> 
> I copied it from
> http://processhacker.svn.sourceforge.net/viewvc/processhacker/2.x/trunk/
> plugins/ExtendedTools/d3dkmt.h?revision=4758&view=markup and made some tiny
> changes to it so the function definitions would compile like in C.

Oh and comment 3 says it came from the Win8 WDK originally.

Comment 13

[Justin Lebar (not reading bugmail)]

We can't use the version copied from ProcessHacker, because it's GPL/LGPL.  You'll need to check the license of the WDK and see if we can use the file directly from there.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to Hugh Nougher [:Hughman] from comment #11)
> (In reply to Justin Lebar [:jlebar] from comment #9)
> > Where did this d3dkmt.h file come from?  It looks like it was copied from
> > somewhere?
> 
> I copied it from
> http://processhacker.svn.sourceforge.net/viewvc/processhacker/2.x/trunk/
> plugins/ExtendedTools/d3dkmt.h?revision=4758&view=markup and made some tiny
> changes to it so the function definitions would compile like in C.

I'm not sure that the WDK headers will be redistributable. I'd suggest making our own header that defines the things we need.

> (In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> > You can get a IDXGIAdapter from IDXGIDevice::GetAdapater()
> 
> There is one thing I keep wondering about... what happens when there is two
> adapters? Would that mean that there needs to be two devices if firefox had
> a window on each? Then what about sli/crossfire setups?

I'm pretty sure we only use one device at a time. I believe the driver hides the sli/crossfire setup behind a single device.

> Another is what happens when the adapter or driver is reset. I have seems
> cases where due to a bug in the driver that win7 would kill the old driver
> and start again causing some programs to loose visual though firefox is
> usually ok. Updating drivers also causes this. When this happens does the
> adapter stay the same?

The adapater should stay the same. But it might be ok for us to reget the adapter everytime the memory stats are needed.

I believe you can use ID3D10Device1 *device = cairo_d2d_device_get_device(mD2DDevice); to do this.

Bas, is this correct?

Comment 15

[Hugh Nougher [:Hughman]]

Attachment: WIP-V0.2

This patch still uses the old d3dkmt.h because I dont know how to get hold of WDK and not knowing what direction to take with it at this stage.

Anyway this update actually given the same dedicated and shared memory reading as Process Explorer on my machine. Yay! It also currently has 4 other reporters which specify the limits of the available GPU memories (which I intend to take out again unless they are wanted).

   31.54 MB -- d3d/dedicated-memory
1,012.43 MB -- d3d/limit-dedicated-memory
2,816.00 MB -- d3d/limit-shared-memory
1,012.43 MB -- d3d/max-dedicated-memory
2,815.25 MB -- d3d/max-shared-memory
    3.71 MB -- d3d/shared-memory

I do wonder if using mozilla::gfx::Factory::GetDirect3D10Device was the right thing to do. It returns a ID3D10Device1 which is not quite a ID3D10Device but it should be the same as cairo_d2d_device_get_device() since that is what is used to set it.

Things left to do include:
- determining if running on win7 or win8.
- determining if the process handle is needed and how to get it.
- wonder what the "Committed GPU Memory" means in Process Explorer and find it if useful.
- work out what the memory reporters should be called and their descriptions for about:memory.

Comment 16

[Hugh Nougher [:Hughman]]

Attachment: WIP-V0.3

This patch is very close to complete. Only the d3dkmt, memory reporter names/descriptions and other hardware/software configuration testing.

I have cleaned up the code to a level that I think is acceptable and renamed the reporters to something simpler. I also added a check that the windows version is a minimum of win7 before starting to collect data because I believe it does not work in earlier version of windows.

The reporters currently look like this:
 31.72 MB -- gpu-dedicated
  3.59 MB -- gpu-shared

I still do not have any real idea of what "Committed GPU Memory" is but from a little research I think its related to a processes commit charge. It also appears to always be <= dedicated + shared. I also still do not know where to get this information so its being left out.

Aa for a minimum testing set I think the following is needed (excluding ones I can do):
- Windows 8
- Win7/8 with two graphics cards
- Win7/8 with integrated graphics

How would that be gone about?

Also I noticed that when graphics acceleration is disabled, this will still add the two reporters to about:memory with values of 0. Is this ok?

I will likely be leaving this alone until some course of action(s) is given.

Comment 17

[Hugh Nougher [:Hughman]]

Do we have any direction to go with d3dkmt.h yet? I have no way to learn of the windows 8 sdk licence so I have to leave that for someone else.

Alternately I could go ahead and attempt removal of as many parts of that header file as possible. Is that a good course of action?

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(In reply to Hugh Nougher [:Hughman] from comment #17)
> Do we have any direction to go with d3dkmt.h yet? I have no way to learn of
> the windows 8 sdk licence so I have to leave that for someone else.
> 
> Alternately I could go ahead and attempt removal of as many parts of that
> header file as possible. Is that a good course of action?

I'll try to ask legal about this or come up with a suggestion.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> (In reply to Hugh Nougher [:Hughman] from comment #17)
> > Do we have any direction to go with d3dkmt.h yet? I have no way to learn of
> > the windows 8 sdk licence so I have to leave that for someone else.
> > 
> > Alternately I could go ahead and attempt removal of as many parts of that
> > header file as possible. Is that a good course of action?
> 
> I'll try to ask legal about this or come up with a suggestion.

It looks like this d3dkmt.h may not be copied from ddk. In this case, we should be able to ask wj32 (the author) if he'd be willing to relicense the file under the tri-license.

Comment 20

[Hugh Nougher [:Hughman]]

I asked wj32 about the header file and this is what he says.

> A while ago someone from the ReactOS project gave me this advice: in general,
> header files aren't copyrighted. You can also read this:
> http://www.winehq.org/pipermail/wine-devel/2001-February/000204.html.
> To address your question more specifically, I did in fact copy the definitions
> from the Windows 8 SDK, so I'm not the right person to ask for permission.
> But as above, I don't think you actually need permission for this kind of usage.
> 
> Now I would like a mention somewhere in the gfxWindowsPlatform.cpp source
> code (e.g. "from Process Hacker") just because figuring out this undocumented
> stuff is a complete pain in the ass! :D

So it appears it will be up to legal or I add a check around this code that the various things in the header are defined (which they will be when compiled against win8 sdk, or thats what I think).


As to the comment for wj32, I added the following. Is this OK?

--- a/gfx/thebes/gfxWindowsPlatform.cpp
+++ b/gfx/thebes/gfxWindowsPlatform.cpp
@@ -223,4 +223,6 @@ GPUAdapterMemoryReporter::CollectReports
     if (D3DKMTQueryStatistics_I && GetDXGIAdapter(&DXGIAdapter))
     {
+        // Most of this block is understood thanks to wj32's work on Process Hacker
+
         DXGI_ADAPTER_DESC adapterDesc;
         D3DKMT_QUERYSTATISTICS queryStatistics;

Comment 21

[Jim Mathies [:jimm]]

(In reply to Hugh Nougher [:Hughman] from comment #17)
> Do we have any direction to go with d3dkmt.h yet? I have no way to learn of
> the windows 8 sdk licence so I have to leave that for someone else.

There should be a license folder with the sdk. I'd bet it's the same tos/license they use in other sdks. Straight copy paste of header text is generally frowned upon. The better solution is to include the header and ifdef the code out if the appropriate sdk isn't available.

Comment 22

[Reuben Morais [:reuben]]

I'm assigning this to Hugh so the bug doesn't show up on searches for new mentored bugs.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jim Mathies [:jimm] from comment #21)
> (In reply to Hugh Nougher [:Hughman] from comment #17)
> > Do we have any direction to go with d3dkmt.h yet? I have no way to learn of
> > the windows 8 sdk licence so I have to leave that for someone else.
> 
> There should be a license folder with the sdk. I'd bet it's the same
> tos/license they use in other sdks. Straight copy paste of header text is
> generally frowned upon. The better solution is to include the header and
> ifdef the code out if the appropriate sdk isn't available.

Currently this header is only in the windows 8 sdk. I don't think there's an easy way to get this sdk yet, so it would be good if we had a solution for building on older platforms, without having to wait for window 8.

Comment 24

[André Ziegler]

the SDK can be downloaded from MSDN (if you have a subscription)

Comment 25

[Hugh Nougher [:Hughman]]

Attachment: Patch V1

This patch has d3dkmt.h removed. I do have it in another patch in my queue as its my only way of testing changes.

It also has an attempt at blindly using d3dkmthk.h if MOZ_WINSDK_TARGETVER > MOZ_NTDDI_WIN7. I'm blind here because I am using Win7SDK and have no MSDN subscription.

Requesting feedback from mentor as there is almost guaranteed to be problems to fix.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Sorry about the delay here. We had major email problems @mozilla.com so I haven't been reading bugmail. I'll try to get feedback asap.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 579595 [details] [diff] [review]
Patch V1

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

This looks really good. Other than a couple of style nits this looks like it would be ready to land. I've asked legal for advice on what we should do about the header, so we can resolve that after this lands.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +107,5 @@
> +#include <winternl.h>
> +
> +extern "C" {
> +#include <d3dkmthk.h>
> +}

Is the extern "C" really needed? Usually it's a header's job to do this. If it doesn't we should probably add a comment to that effect.

@@ +232,5 @@
> +    if (winVers < gfxWindowsPlatform::kWindows7) 
> +        return NS_OK;
> +    
> +    if (gdi32Handle = LoadLibrary(TEXT("gdi32.dll")))
> +        D3DKMTQueryStatistics_I = (PFND3DKMT_QUERYSTATISTICS)GetProcAddress(gdi32Handle, "D3DKMTQueryStatistics");

The convention in this file is for the return value of GetProcAddresss to go into a variable named like 'createDWriteFactory'

@@ +260,5 @@
> +                queryStatistics.AdapterLuid = adapterDesc.AdapterLuid;
> +                queryStatistics.QuerySegment.SegmentId = i;
> +                
> +                if (NT_SUCCESS(D3DKMTQueryStatistics_I(&queryStatistics)))
> +                {

This file has the '{' on the same line as the 'if'. Same with 'for'.

Comment 28

[Hugh Nougher [:Hughman]]

Attachment: Patch V2

Here is a new patch with the changes from the feedback.

(In reply to Hugh Nougher [:Hughman] from comment #16)
> I still do not have any real idea of what "Committed GPU Memory" is but from
> a little research I think its related to a processes commit charge. It also
> appears to always be <= dedicated + shared. I also still do not know where
> to get this information so its being left out.

I found a use for it the other day so I asked wj32 about where he thought it was and therefore we now have a gpu-committed reporter.

The use I found for it was with WebGL/OpenGL where the memory it allocates seems to add to the commited bytes while dedicated and shared can stay very low in comparison. This is just from my observation while viewing mapcrunch (see bug 643651) so I may be wrong. It should have its uses in any case.

Again, this patch has not been tested with Win8 SDK so it may not work at all.

Comment 29

[Kelsey Gilbert [:jgilbert]]

Here's the try push:
https://tbpl.mozilla.org/?tree=Try&rev=640c6ea33e7f

Comment 30

[Hugh Nougher [:Hughman]]

Comment on attachment 581940 [details] [diff] [review]
Patch V2

Jeff: Thanks for pushing to try.

I think the run looks fine. Moth leak appears to be cache related and other platforms should be unaffected anyway.

As per https://wiki.mozilla.org/Platform/Memory_Reporting its a co-review.

Comment 31

[Nicholas Nethercote [inactive]]

Comment on attachment 581940 [details] [diff] [review]
Patch V2

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

There are several XXX comments, will those things be fixed before landing?

jrmuizel suggested sorting out the legal stuff after landing, but that sound backwards to me.

This is on the right track but I'm giving r- because I'd like to see it again before landing.  Thanks!

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +89,5 @@
> +#include "nsMemory.h"
> +#endif
> +
> +/**
> + * XXX below should be >= MOZ_NTDDI_WIN8 or such which is not defined yet

Will this be fixed before the patch lands?

@@ +188,5 @@
>  );
>  #endif
>  
> +#ifdef ENABLE_GPU_MEM_REPORTER
> +class GPUAdapterMemoryReporter : public nsIMemoryMultiReporter {

I'd call it GPUAdapterMultiReporter.

@@ +215,5 @@
> +NS_IMPL_ISUPPORTS1(GPUAdapterMemoryReporter, nsIMemoryMultiReporter)
> +
> +NS_IMETHODIMP
> +GPUAdapterMemoryReporter::CollectReports(nsIMemoryMultiReporterCallback* aCb,
> +                                         nsISupports* aClosure)

You could put this inline in GPUAdapterMemoryReporter if you wanted.

@@ +231,5 @@
> +    
> +    winVers = gfxWindowsPlatform::WindowsOSVersion(&buildNum);
> +    
> +    // GPU memory reporting is not available before Windows 7
> +    if (winVers < gfxWindowsPlatform::kWindows7) 

Can this be false if ENABLE_GPU_MEM_REPORTER is defined?

@@ +320,5 @@
> +                  NS_LITERAL_CSTRING("gpu-shared"),
> +                  nsIMemoryReporter::KIND_OTHER,
> +                  nsIMemoryReporter::UNITS_BYTES,
> +                  sharedBytesUsed,
> +                  NS_LITERAL_CSTRING("Application memory allocated by D3D for GPU use."),

The names and descriptions are a bit unclear.

What do you mean by "Application memory"?  In the browser's address space?  If so, I'd say "In-process memory allocated...".  I'd likewise add "In-process" to the start of the description for "gpu-committed", and "Out-of-process" to the start of the description for "gpu-dedicated".

What's the difference between "gpu-committed" and "gpu-shared"?  The latter is only D3D, but the former can be OpenGL too?

Should "d3d" be in any of the names?  IIRC we have "d2d" in the names of some of the existing reporters.

Do these three measurements have any overlap, i.e. could any memory be reported in more than one of them?  It's ok if they do, because they have KIND_OTHER, but it's not clear from the descriptions.

@@ +367,5 @@
>  
>  gfxWindowsPlatform::~gfxWindowsPlatform()
>  {
> +#ifdef ENABLE_GPU_MEM_REPORTER
> +    NS_UnregisterMemoryMultiReporter(new GPUAdapterMemoryReporter);

That's completely wrong :)  When you try to unregister, you're creating a new multi-reporter, and NS_UnregisterMemoryMultiReporter will look for it in its list and fail to find it because it hasn't been registered previously.  Meanwhile the existing multi-reporter won't be removed.

Instead, when you create the multi-reporter and register it, you need to save its pointer in a member.  Then you pass that member to NS_UnregisterMemoryMultiReporter.  Look at |mHunspellReporter| in extensions/spellcheck/hunspell/src/mozHunspell.{h,cpp} for an existing example;  it's a normal reporter instead of a multi-reporter, but it's much the same.

Comment 32

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #31)
> Comment on attachment 581940 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 581940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are several XXX comments, will those things be fixed before landing?

First one replies on Win8 being released. The second one could be changed to a normal comment now if someone can explain why I dont have NTSTATUS defined from windows.h. The third relies on knowing what is in the win8 SDK which I have no access to.

> jrmuizel suggested sorting out the legal stuff after landing, but that sound
> backwards to me.
> 
> This is on the right track but I'm giving r- because I'd like to see it
> again before landing.  Thanks!
> 
> ::: gfx/thebes/gfxWindowsPlatform.cpp
> @@ +89,5 @@
> > +#include "nsMemory.h"
> > +#endif
> > +
> > +/**
> > + * XXX below should be >= MOZ_NTDDI_WIN8 or such which is not defined yet
> 
> Will this be fixed before the patch lands?

I think the correct place to define that is
http://dxr.mozilla.org/mozilla/mozilla-central/configure.in.html#l1006
and that will need to rely on what version number Microsoft give win8 for release. Last I checked there was at best a rumour that it will be 6.2.

> @@ +188,5 @@
> >  );
> >  #endif
> >  
> > +#ifdef ENABLE_GPU_MEM_REPORTER
> > +class GPUAdapterMemoryReporter : public nsIMemoryMultiReporter {
> 
> I'd call it GPUAdapterMultiReporter.

Sounds good.

> @@ +215,5 @@
> > +NS_IMPL_ISUPPORTS1(GPUAdapterMemoryReporter, nsIMemoryMultiReporter)
> > +
> > +NS_IMETHODIMP
> > +GPUAdapterMemoryReporter::CollectReports(nsIMemoryMultiReporterCallback* aCb,
> > +                                         nsISupports* aClosure)
> 
> You could put this inline in GPUAdapterMemoryReporter if you wanted.

How would I go about doing that? Id guess that we remove NS_DECL_NSIMEMORYMULTIREPORTER and leave NS_IMETHODIMP. Is this correct?
(I was originally going to have it inline but couldnt find an example of doing so)

> @@ +231,5 @@
> > +    
> > +    winVers = gfxWindowsPlatform::WindowsOSVersion(&buildNum);
> > +    
> > +    // GPU memory reporting is not available before Windows 7
> > +    if (winVers < gfxWindowsPlatform::kWindows7) 
> 
> Can this be false if ENABLE_GPU_MEM_REPORTER is defined?

You can compile with win7 SDK for vista at least so I assume it will be the same for win8 SDK. From my test on Vista all the values come out as zero so not showing the entries in about:memory makes sense. I also guess that it might crash in XP and earlier.

> @@ +320,5 @@
> > +                  NS_LITERAL_CSTRING("gpu-shared"),
> > +                  nsIMemoryReporter::KIND_OTHER,
> > +                  nsIMemoryReporter::UNITS_BYTES,
> > +                  sharedBytesUsed,
> > +                  NS_LITERAL_CSTRING("Application memory allocated by D3D for GPU use."),
> 
> The names and descriptions are a bit unclear.
> 
> What do you mean by "Application memory"?  In the browser's address space? 
> If so, I'd say "In-process memory allocated...".  I'd likewise add
> "In-process" to the start of the description for "gpu-committed", and
> "Out-of-process" to the start of the description for "gpu-dedicated".
> 
> What's the difference between "gpu-committed" and "gpu-shared"?  The latter
> is only D3D, but the former can be OpenGL too?

/me runs into borderline knowledge for this.
Here is my guesses with as much proof as I can round up.

gpu-committed takes the value of SystemMemory.BytesAllocated. Since the lookup to get this information uses a DXGI Adapter and a Process Handle to restrict it we know these are for something in the graphics system. Right next to it there is SystemMemory.BytesReserved which sounds familiar with windows VirtualAlloc which its 3rd parameter documentation comes in handy here.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366887%28v=vs.85%29.aspx

So gpu-committed is really "Allocat[ed] physical storage in memory or in the paging file on disk for the specified reserved memory pages". My guess from reading around and observation in Process Explorer is its likely that this counts all memory used by non-driver-generated objects in dedicated or shared memory. This would so that if memory contention happens it can page the textures/shaders to disk, though I do not think anyone would be happy if that really happened. Again it mostly guessing.

The OpenGL part is only a guess. While looking at mapcrunch I see the following values:
gpu-committed = 900MB
gpu-dedicated = 270MB (this has a range of about 70-900MB and drops when OOM would happen without affecting committed or shared)
gpu-shared = 20MB

Maybe a better description could be thought up given this.

> Should "d3d" be in any of the names?  IIRC we have "d2d" in the names of
> some of the existing reporters.

Memory used by d2d should also be in these values. I believe the values are coming from the DXGI level of the windows graphics system which encompasses the driver and D3D/D2D areas (someone correct me if I am wrong, its all observation/guesses). I am removing the references to D3D from the reporter descriptions.

> Do these three measurements have any overlap, i.e. could any memory be
> reported in more than one of them?  It's ok if they do, because they have
> KIND_OTHER, but it's not clear from the descriptions.
> 

gpu-committed includes some of gpu-dedicated, some of gpu-shared and some other stuff. Explanation before may have more answers.

> @@ +367,5 @@
> >  
> >  gfxWindowsPlatform::~gfxWindowsPlatform()
> >  {
> > +#ifdef ENABLE_GPU_MEM_REPORTER
> > +    NS_UnregisterMemoryMultiReporter(new GPUAdapterMemoryReporter);
> 
> That's completely wrong :)  When you try to unregister, you're creating a
> new multi-reporter, and NS_UnregisterMemoryMultiReporter will look for it in
> its list and fail to find it because it hasn't been registered previously. 
> Meanwhile the existing multi-reporter won't be removed.
> 
> Instead, when you create the multi-reporter and register it, you need to
> save its pointer in a member.  Then you pass that member to
> NS_UnregisterMemoryMultiReporter.  Look at |mHunspellReporter| in
> extensions/spellcheck/hunspell/src/mozHunspell.{h,cpp} for an existing
> example;  it's a normal reporter instead of a multi-reporter, but it's much
> the same.

...wow. How did I miss that. Its so obvious.


Really this is all about quite a lot of not-yet-documented stuff which although it gives me a headache to explain and has many unknowns around it, they will come in very useful when bug reporter gives an about:memory paste which has high private/resident and low heap/explicit which can indicate lots of shared use.

An example is attachment 557086 [details] which would also have said at least 1GB gpu-shared.

Comment 33

[Nicholas Nethercote [inactive]]

> > You could put this inline in GPUAdapterMemoryReporter if you wanted.
> 
> How would I go about doing that? Id guess that we remove
> NS_DECL_NSIMEMORYMULTIREPORTER and leave NS_IMETHODIMP. Is this correct?

It becomes NS_IMETHOD when inline.  See http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#146 for an example.

> > The names and descriptions are a bit unclear.
> 
> /me runs into borderline knowledge for this.
> Here is my guesses with as much proof as I can round up.

Thanks for the explanation.  I'll leave the final names and descriptions up to you, make them as good as you can! :)

Comment 34

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicholas Nethercote [:njn] from comment #31)
> Comment on attachment 581940 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 581940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are several XXX comments, will those things be fixed before landing?
> 
> jrmuizel suggested sorting out the legal stuff after landing, but that sound
> backwards to me.

The questionable header is not included in the current patch so we don't need to wait. At the same this won't end in actual builds until we solve the problem so there's no rush either.

Comment 35

[Hugh Nougher [:Hughman]]

njn: Did something change with nsIMemoryMultiReporter recently?

Its currently telling me that GetExplicitNonHeap(PRInt64 *aExplicitNonHeap) needs to be implemented because its abstract.

Comment 36

[Justin Lebar (not reading bugmail)]

(In reply to Hugh Nougher [:Hughman] from comment #35)
> njn: Did something change with nsIMemoryMultiReporter recently?
> 
> Its currently telling me that GetExplicitNonHeap(PRInt64 *aExplicitNonHeap)
> needs to be implemented because its abstract.

Not too recently, but yes, you do need to implement that method now.  :)

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 581940 [details] [diff] [review]
Patch V2

Dropping review request until there's a new patch

Comment 38

[Nicholas Nethercote [inactive]]

Hugh, have you had a chance to make progress on this?  Seems like you weren't that far off getting the r+.

Comment 39

[Hugh Nougher [:Hughman]]

Attachment: Patch V3

After being lazy for a few weeks here is an updated patch. I believe there is no point running another try run on this since if you diff with the last patch only things that are ifdef'd out for win8 sdk have been changed.

I still think it would be a good idea to have someone with WIn8 SDK compile with this to make certain it at least compiles much less works correctly before it gets pushed (whenever that happens).

I have also made a comment over on bug 687213 about getting the Win8 SDK detection in the configure stuff.

Comment 40

[Hugh Nougher [:Hughman]]

Attachment: D3DKMT to place over V3 (not for pushing)

For those who compile with Win7 SDK (like me) so they can test with these reporters.

Comment 41

[Nicholas Nethercote [inactive]]

Comment on attachment 590629 [details] [diff] [review]
Patch V3

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

I don't love that the meaning of these measurements is so unclear, but I can understand that getting clear explanations is difficult on this kind of deep system Windows stuff.  r=me with the singleton removed, as I describe below.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +214,5 @@
> +    // Sets up the reporters needed
> +    static void Setup()
> +    {
> +        // Supposed to be one reporter per DXGIAdapter which there obviously is not
> +        // since it was deemed not nessessary. (bug 689870 comment 14)

s/nessessary/necessary/

@@ +215,5 @@
> +    static void Setup()
> +    {
> +        // Supposed to be one reporter per DXGIAdapter which there obviously is not
> +        // since it was deemed not nessessary. (bug 689870 comment 14)
> +        NS_ABORT_IF_FALSE(sReporterInstance == 0, "GPUAdapterMultiReporter::Setup() can only be run once!");

s/0/nsnull/

But using a singleton is overkill here.  You should just:

- Add a |nsIMemoryMultiReporter*| member to |gfxWindowsPlatform|.

- Create and register it in gfxWindowsPlatform(), where you currently call Setup().

- Unregister it in ~gfxWindowsPlatform() where you currently call TakeDown().  (After unregistering its refcount will drop to zero and it'll be freed.)

This would be like the mHunspellReporter example I mentioned in comment 31.

@@ +320,5 @@
> +                      nsIMemoryReporter::KIND_OTHER,
> +                      nsIMemoryReporter::UNITS_BYTES,
> +                      committedBytesUsed,
> +                      NS_LITERAL_CSTRING("Allocated physical storage in memory or in the paging file "
> +                                         "on disk for this process by the windows graphics system."),

We use "committed" in a couple of other places in about:memory, so I think "Memory committed by the Windows graphics system." would suffuce.

@@ +328,5 @@
> +                      nsIMemoryReporter::KIND_OTHER,
> +                      nsIMemoryReporter::UNITS_BYTES,
> +                      dedicatedBytesUsed,
> +                      NS_LITERAL_CSTRING("Out-of-process memory allocated for this process in a "
> +                                         "physical GPU Adapter's dedicated memory."),

s/Adapter/adapter/, and can "dedicated" be removed without loss of clarity?

@@ +335,5 @@
> +                      NS_LITERAL_CSTRING("gpu-shared"),
> +                      nsIMemoryReporter::KIND_OTHER,
> +                      nsIMemoryReporter::UNITS_BYTES,
> +                      sharedBytesUsed,
> +                      NS_LITERAL_CSTRING("In-process memory allocated for windows graphics system use."),

This description doesn't have the word "shared" in it, which surprises me... surely it should?

And how does this relate to "gpu-committed"?  Is it a subset?

@@ +345,5 @@
> +    // nsIMemoryMultiReporter abstract method implementation
> +    NS_IMETHOD
> +    GetExplicitNonHeap(PRInt64 *aExplicitNonHeap)
> +    {
> +        // This reporter doesn't do any heap or non-heap measurements.

No need for the "heap or", it's only non-heap measurements that matter in this function.

Comment 42

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #41)
> But using a singleton is overkill here.

Ill do that shortly. I came up with that structure because there would need to be a reporter for each physical adapter in a machine but at the moment we are only using one adapter and not even passing in which adapter to collect information on.

> s/Adapter/adapter/, and can "dedicated" be removed without loss of clarity?

I removed dedicated also. The GPU adapter's memory will often be on the adapter itself. Windows does allocate a small amount of main memory for adapters with no on-board memory that is dedicated to their use which I think is still covered by this.

> This description doesn't have the word "shared" in it, which surprises me...
> surely it should?

If you want it in there please make a suggestion. Here is some points to consider:
- gpu-shared is shared in the sense of the main memory being shared with another component of the computer, in this case the GPU.
- the memory allocated may or may not be accessible by the process itself depending on how the resource stored has been set up. In either case I think you have to ask the Windows graphics system for access to the memory while you use it.
- although its measured as part of in-process memory the process has no control over the allocation. The allocation happens inside D2D/D3D/OpenGL/DXGI or the graphics driver.

> And how does this relate to "gpu-committed"?  Is it a subset?

Maybe... maybe not... I have not seen anything which points one way or the other.

Comment 43

[Nicholas Nethercote [inactive]]

(In reply to Hugh Nougher [:Hughman] from comment #42)
> 
> > This description doesn't have the word "shared" in it, which surprises me...
> > surely it should?
> 
> - gpu-shared is shared in the sense of the main memory being shared with
> another component of the computer, in this case the GPU.

Oh, I see.  In that case, the description should say that :)  E.g. "In-process memory that is shared with the GPU."

Comment 44

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #43)
> "In-process memory that is shared with the GPU."

I don't think that this is good enough. Although most of it will likely be used by the GPU at some point, I think there would be some memory which is only used by the Windows graphics system for whatever reason. I have put your suggestion in there for the moment anyway.

Comment 45

[Hugh Nougher [:Hughman]]

Attachment: Patch V3.1

This may need a try run because it moves the include of nsIMemoryReporter.h into the header and there is the extra pointer in gfxWindowsPlatform.

The pointer in gfxWindowsPlatform does not have an ifdef around it because its neater without and its just one tiny pointer. To fix I would have to move the Win8 SDK detection stuff into the header.

Comment 46

[Nicholas Nethercote [inactive]]

> This may need a try run because it moves the include of nsIMemoryReporter.h
> into the header and there is the extra pointer in gfxWindowsPlatform.

Ah, but because you're only using a |nsIMemoryMultiReporter*| in gfxWindowsPlatform.h, you can just do a forward declaration:

  class nsIMemoryMultiReporter;

And then you don't need to #include nsIMemoryReporter.h!

Comment 47

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 591011 [details] [diff] [review]
Patch V3.1

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +98,5 @@
> +#ifdef ENABLE_GPU_MEM_REPORTER
> +/**
> + * XXX to get NTSTATUS defined. No idea why I cant get it from windows.h
> + */
> +#include <winternl.h>

Is this for NT_SUCCESS? I don't really like including this but would be ok with it, if there's no other way.

::: gfx/thebes/gfxWindowsPlatform.h
@@ +283,5 @@
>  
>      // TODO: unify this with mPrefFonts (NB: holds families, not fonts) in gfxPlatformFontList
>      nsDataHashtable<nsCStringHashKey, nsTArray<nsRefPtr<gfxFontEntry> > > mPrefFonts;
> +
> +    nsIMemoryMultiReporter* mGPUAdapterMultiReporter;

Yeah, just forward declare this class.

Comment 48

[Hugh Nougher [:Hughman]]

Attachment: Patch V3.2

Been lazy again. Here is the new patch with the forward declaration and the XXX removed from before the winternl.h include.

I am keeping the include because its likely more useful how it is. NT_SUCCESS mostly just checks the NTSTATUS is >= 0, where NTSTATUS is a LONG. So the alternate would likely be to cast it to LONG then compare with >= 0 which is a little nasty to read. Also the include has been used before in files such as nsWindowsDllBlocklist.

r+ carried forward from V3 & V3.1.

Again a try run should not be needed since the only change outside of the IFDEFs is the new property in class gfxWindowsPlatform and it compiles locally. Still no checks with Win8 SDK though so it may not work at all.

Comment 49

[Nicholas Nethercote [inactive]]

I think this patch is ready to land?  If so, I can do it.

Comment 50

[Hugh Nougher [:Hughman]]

Yes it is ready to land if you agree with what I said at the end of comment 48.

Comment 51

[Nicholas Nethercote [inactive]]

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

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d3298d4a80c4

Comment 53

[André Ziegler]

Win8 Consumer Preview/Beta SDK/WDK no longer includes the definition in the d3dkmt.h. So add the d3dkmt.h to the source and activate it for Win7/8.

Comment 54

[Jeff Muizelaar [:jrmuizel]]

Is it possible for someone who has access to the SDK that did include the definitions post the EULA and REDIST.txt to this bug or someplace else accessible?

Comment 55

[André Ziegler]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #54)
> Is it possible for someone who has access to the SDK that did include the
> definitions post the EULA and REDIST.txt to this bug or someplace else
> accessible?

There is no redist.txt.

Comment 56

[André Ziegler]

any news? when will it be activated?

Comment 57

[Hugh Nougher [:Hughman]]

I just realised that due to comment 53 this should not have been closed.

So we really are still waiting on news of d3dkmt.h.

Comment 58

[André Ziegler]

same applies to Windows 8 Release Preview/RC WDK. The d3dkmthk.h is still included, but the definition are again not included.

When I checked the header file from Build 8102 I can see this comment

// This section will be removed from LDDM DDK
EXTERN_C _Check_return_ NTSTATUS APIENTRY D3DKMTQueryStatistics(_In_ CONST D3DKMT_QUERYSTATISTICS*);
// 

so MS never wanted that the definition are published.

Comment 59

[André Ziegler]

The Win8 RTM WDK again doesn't contain the definition in the d3dkmthk.h.

Comment 60

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi there Hugh, are you still working on this bug? I see things got a bit complicated, and it's been a while. If you're not sure of its status I will go find the bug mentor in irc and see if we can figure out what's up. Thanks!

Comment 61

[Hugh Nougher [:Hughman]]

Wow... Its been a while since I looked at this.

I few months back I learnt that I had to contact licensing (something I definitely did not know before I read a random post by someone else which explained it). I got a reply from Grev which basically said recreating the header file was the best way forward. Since then I have not had any real free time. However since you poked/reminded me on a public holiday I now have done the work.

There will be new patch and feedback requests coming soon to see if I have done it right.

Comment 62

[Hugh Nougher [:Hughman]]

Attachment: Diagram that helps explain structure of d3dkmt

I created this rough diagram to help me understand the layout of the structures while I was recreating it. It might also help future developers find other parts they are interested in for implementation.

(On a side note this SVG scrolls much faster in IE10 than Firefox 23.0a1)

Comment 63

[Hugh Nougher [:Hughman]]

Attachment: D3DKMTQS Patch V4

And here is the patch. Since I now use Win8 it has been tested to work on Win8.

Gerv: is this good enough as a rewrite? Should I change the structure names more? Or maybe I have gone overboard by eliminating everything I don't use?

For the wider audience, is there a better way to reduce the depth of these structures? Possibly some of these structures are implemented somewhere else that I do not know about?

Comment 64

[Gervase Markham [:gerv]]

:Hughman: as long as you didn't start your hacking with a copy of the encumbered/tainted code, or copy and paste from it, it's good enough. :-)

If you did either of these things, then a) you should have asked what you could and couldn't do before you started, and b) it needs to be done again, probably by someone else :-|

Gerv

Comment 65

[Hugh Nougher [:Hughman]]

Attachment: D3DKMTQS Patch V4.1

Updated patch. Human readable variable names. Less fillers.
Will ask someone to push to try for a run though.

(In reply to Gervase Markham [:gerv] from comment #64)
> :Hughman: as long as you didn't start your hacking with a copy of the
> encumbered/tainted code, or copy and paste from it, it's good enough. :-)

No starting with original or copying. The diagram was used to make it so I could just create what I needed (which was a very small portion of it and hence the amount of fillers). A bit tired/lazy when I did it so many of the names of variables were very similar.

Comment 66

[Hugh Nougher [:Hughman]]

I have just noticed a lot of tab/space swapping in the header so after review and try I will upload another patch with all tabs as 4 spaces.

Comment 67

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 744065 [details] [diff] [review]
D3DKMTQS Patch V4.1

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

A couple of nits, but otherwise this looks ok.

::: gfx/thebes/d3dkmtQueryStatistics.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

It's probably worth including a comment about where the information for this header came from.

@@ +86,5 @@
> +{
> +    ULONG Filler[2];
> +	struct {
> +		ULONGLONG BytesAllocated;
> +		

There's some trailing space here that we might as well get rid of.

@@ +96,5 @@
> +
> +typedef struct _D3DKMTQS_PROCESS_SEGMENT_INFO
> +{
> +    ULONGLONG BytesCommitted;
> +    

And here.

Comment 68

[Hugh Nougher [:Hughman]]

https://tbpl.mozilla.org/?tree=Try&rev=2714a0fa4ef0

Comment 69

[Hugh Nougher [:Hughman]]

Attachment: D3DKMTQS Patch V4.2

Fixes of white-space and added comment at top.
Carried forward r+

Comment 70

[Ryan VanderMeulen [:RyanVM]]

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

Thanks for the patch, Hugh!

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1fe50632492

Comment 72

[Jacek Caban]

This broke mingw build, that doesn't support __in and __out annotations due to conflicts with libstdc+++. I landed a fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85249bddf0a6

Comment 73

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85249bddf0a6