David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 8 years ago

Right now about:memory doesn't show any information about the GPU process. It's not that substantial since the GPU process is so light, but we should probably get it working anyway.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 8 years ago

This factors MemoryReportRequestParent/Child into a separate header/source file, so the guts can be shared across multiple process types. There's no functional changes in this patch.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 8 years ago

This changes MemoryReportRequestChild so that it doesn't need a ContentChild instance to get the process name. Instead, ContentChild must construct the request with a process name up front.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 8 years ago

It's difficult to share protocols like PMemoryReportRequest with the GPU process sine the GPU process has an inverted Child/Parent relationship. "GPUChild" is in the parent process, for example. IPDL really cannot deal with this yet. PCrashReporter had a similar problem.

The easiest thing to do is to just remove the protocol and move its methods into top-level actors. That's what this patch does. All messages on PMemoryReportRequest are moved into PContent. The constructor becomes "RequestMemoryReport", and the destructor becomes "FinishMemoryReport".

MemoryReportRequestParent becomes MemoryReportRequestHost. ContentParent allocates one every time it starts a new request.

MemoryReportRequestChild becomes MemoryReportRequestClient. ContentChild doesn't explicitly interact with it, instead, it just defers to a new static helper. This is so the minimize flag code can be shared across process types.

The biggest effect from this is that previously, PMemoryReportRequest actors could pile up if you kept clicking the "Measure" button in about:memory. That's no longer the case, as each ContentParent immediately destroys old requests. As a consequence we have to associate a generation number with MemoryReport messages, otherwise we don't know which generation a message was attached to.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 4 • 8 years ago

Right now nsMemoryReportingManager assumes every process is a ContentParent. In order to support more process types we have to factor out what it needs. I did that here into a "MemoryReportingProcess" class, which ContentParent now implements. (The name is terrible, better one is welcome.)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 8 years ago

This moves MaybeFileDesc from PContent to MemoryReportingTypes.ipdlh. We need to use the struct in PGPU, and IPDL doesn't let you import structs/unions from non-header files.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 8 years ago

This modifies the PGPU protocol to implement the same memory reporting functions as the PContent protocol. It should look identical since it basically does the same thing. (If we extend this to more process types we could factor it out into a base class, I guess.)

The reason we don't have GPUChild inherit from MemoryReportingProcess is that GPUChild is not refcounted. So GPUProcessManager provides a refcounted wrapper instead.

The nsComponentManager change is because the GPU process doesn't have dynamic XPCOM modules, so this would crash when generating a memory report.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 7 • 8 years ago

These are the final nsMemoryReportingManager changes needed to show the GPU process. The manager change makes sure to add the GPU process to the pending process list. The MemoryReportRequest changes let us send messages through the right singleton when in the GPU process.

	Ryan Hunt [:rhunt]
	Comment 8 • 8 years ago

Comment on attachment 8816056 [details] [diff] [review]
part 6, GPU process changes

This all looks good. Just a few comments.

>+  // If a GPU process is present, create a MemoryReportingProcess object.
>+  // Otherwise, return null.
>+  RefPtr<MemoryReportingProcess> GetProcessMemoryReporter();
>+

This comment doesn't seem accurate. The code will return a MemoryReportingProcess whether or not a GPU process is present. So either the function or the comment should change.

>+{
>+  nsPrintfCString processName("GPU (pid %u)", getpid());
>+

I think getpid() returns pid_t which can be anything, so to suppress warnings it might be nice to put an explicit cast around it to unsigned. I'm not 100% on this though.

>+  gfxPlatform::RegisterMemoryReporters();
>+  gfxWindowsPlatform::DoStuff();
> #if defined(XP_WIN)

I don't think gfxWindowsPlatform::DoStuff() is a real function :)

	Eric Rahm [:erahm]
	Comment 9 • 8 years ago

Comment on attachment 8816051 [details] [diff] [review]
part 2, remove ContentChild dependency

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

::: dom/ipc/MemoryReportRequest.h
@@ +41,5 @@
>    NS_DECL_ISUPPORTS
>  
>    MemoryReportRequestChild(bool aAnonymize,
> +                           const MaybeFileDesc& aDMDFile,
> +                           const nsCString& aProcessString);

nit: nsACstring

	Eric Rahm [:erahm]
	Comment 10 • 8 years ago

Comment on attachment 8816053 [details] [diff] [review]
part 3, rm PMemoryReportRequest

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

::: dom/ipc/ContentChild.h
@@ +48,5 @@
>  class PStorageChild;
>  class ClonedMessageData;
>  class TabChild;
>  class GetFilesHelperChild;
> +class MemoryReportRequestClient;

Is this necessary?

	Eric Rahm [:erahm]
	Comment 11 • 8 years ago

Comment on attachment 8816054 [details] [diff] [review]
part 4, factor ContentParent out of nsMemoryReportingManager

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

::: xpcom/base/MemoryReportingProcess.h
@@ +16,5 @@
> +} // namespace dom
> +
> +// Top-level process actors should implement this to integrate with
> +// nsMemoryReportManager.
> +class MemoryReportingProcess

Maybe ProcessMemoryReporter? I don't feel strongly about this.

@@ +23,5 @@
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef() = 0;
> +  NS_IMETHOD_(MozExternalRefCountType) Release() = 0;
> +
> +  // Return true if the process is still alive, false otherwise.
> +  virtual bool IsAlive() const = 0;

This almost feels like it should be in a base class, but that's probably overkill.

	Eric Rahm [:erahm]
	Comment 12 • 8 years ago

Comment on attachment 8816056 [details] [diff] [review]
part 6, GPU process changes

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

::: gfx/ipc/GPUChild.h
@@ +13,5 @@
>  
>  namespace mozilla {
>  namespace ipc {
>  class CrashReporterHost;
> +} // namespace pic

nit: ipc

@@ +25,5 @@
>  class GPUChild final
>    : public PGPUChild,
>      public gfxVarReceiver
>  {
> +  typedef mozilla::dom::MemoryReportRequestHost MemoryReportRequestHost;

Could this just be a using |mozilla::dom::MemoryReportRequestHost|?

::: gfx/ipc/GPUParent.cpp
@@ +97,5 @@
>    gfxPlatform::InitNullMetadata();
>    // Ensure our Factory is initialised, mainly for gfx logging to work.
>    gfxPlatform::InitMoz2DLogging();
> +  gfxPlatform::RegisterMemoryReporters();
> +  gfxWindowsPlatform::DoStuff();

This could probably use a comment. Also should it be in XP_WIN?

@@ +369,5 @@
> +                                   const bool& aAnonymize,
> +                                   const bool& aMinimizeMemoryUsage,
> +                                   const MaybeFileDesc& aDMDFile)
> +{
> +  nsPrintfCString processName("GPU (pid %u)", getpid());

Is getpid reliable on all platforms? I feel like we have some sort of xplat way of getting it.

::: gfx/ipc/GPUProcessManager.h
@@ +140,5 @@
>    base::ProcessId GPUProcessPid();
>  
> +  // If a GPU process is present, create a MemoryReportingProcess object.
> +  // Otherwise, return null.
> +  RefPtr<MemoryReportingProcess> GetProcessMemoryReporter();

I think we generally prefer to return |already_AddRefed|

	Eric Rahm [:erahm]
	Comment 13 • 8 years ago

Comment on attachment 8816057 [details] [diff] [review]
part 7, nsMemoryReportingManager changes

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

\o/

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1719,5 @@
> +  }
> +
> +  if (gfx::GPUProcessManager* gpu = gfx::GPUProcessManager::Get()) {
> +    if (RefPtr<MemoryReportingProcess> proc = gpu->GetProcessMemoryReporter()) {
> +      s->mChildrenPending.AppendElement(proc);

nit: proc.forget()

	Eric Rahm [:erahm]
	Comment 14 • 8 years ago

For a final patch I think we should add a test for this that makes sure GPU data is reported. There are some about:memory specific tests under toolkit/aboutMemory, but I think a simple xpcshell test that does whatever setup is need to create a gpu process and calls into |nsIMemoryReporterManager| would probably suffice.

Also have you tossed this at try yet on all platforms?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 8 years ago

(In reply to Ryan Hunt [:rhunt] from comment #8)
> Comment on attachment 8816056 [details] [diff] [review]
> part 6, GPU process changes
> 
> >+  gfxPlatform::RegisterMemoryReporters();
> >+  gfxWindowsPlatform::DoStuff();
> > #if defined(XP_WIN)
> 
> I don't think gfxWindowsPlatform::DoStuff() is a real function :)

Yep, sorry, this was test garbage.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 16 • 8 years ago

(In reply to Eric Rahm [:erahm] from comment #12)
> Comment on attachment 8816056 [details] [diff] [review]
> part 6, GPU process changes
> 
> @@ +25,5 @@
> >  class GPUChild final
> >    : public PGPUChild,
> >      public gfxVarReceiver
> >  {
> > +  typedef mozilla::dom::MemoryReportRequestHost MemoryReportRequestHost;
> 
> Could this just be a using |mozilla::dom::MemoryReportRequestHost|?

I just tried to see if it would work, but it didn't :(

> @@ +369,5 @@
> > +                                   const bool& aAnonymize,
> > +                                   const bool& aMinimizeMemoryUsage,
> > +                                   const MaybeFileDesc& aDMDFile)
> > +{
> > +  nsPrintfCString processName("GPU (pid %u)", getpid());
> 
> Is getpid reliable on all platforms? I feel like we have some sort of xplat
> way of getting it.

I actually copied this from ContentParent.cpp. Looks like there's a header for it on Windows.

> ::: gfx/ipc/GPUProcessManager.h
> @@ +140,5 @@
> >    base::ProcessId GPUProcessPid();
> >  
> > +  // If a GPU process is present, create a MemoryReportingProcess object.
> > +  // Otherwise, return null.
> > +  RefPtr<MemoryReportingProcess> GetProcessMemoryReporter();
> 
> I think we generally prefer to return |already_AddRefed|

I try to avoid it these days... modern C++11 compilers will move the reference, and this isn't that critical a path.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 8 years ago

Part 6 cleaned up based on comments.

	Nicholas Nethercote [inactive]
	Comment 18 • 8 years ago

(In reply to David Anderson [:dvander] from comment #4)
> Created attachment 8816054 [details] [diff] [review]
> part 4, factor ContentParent out of nsMemoryReportingManager
> 
> Right now nsMemoryReportingManager assumes every process is a ContentParent.

Nit: the class is called nsMemoryReporterManager. This is relevant to part 4 and part 7.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 19 • 8 years ago

This exposes a function on nsIGfxInfo to start and stop the GPU process. To get this to work I had to tweak the gfxConfig order a bit, so that a lack of e10s wasn't as fatal.

I also had to fix a bug in GPUChild::ActorDestroy where we logged the process name as "g" in telemetry. It hit an nsDependentCString assert in debug builds.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 20 • 8 years ago

This launches the GPU process, checks that there are any reports at all for it, then tears it down.

	Matt Woodrow (:mattwoodrow)
	Comment 21 • 8 years ago

Comment on attachment 8830130 [details] [diff] [review]
part 8, xpcshell control for gpu process

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

::: widget/GfxInfoBase.cpp
@@ +1477,5 @@
> +GfxInfoBase::ControlGPUProcessForXPCShell(bool aEnable, bool *_retval)
> +{
> +  gfxPlatform::GetPlatform();
> +
> +  printf_stderr("Proc type: %d\n", XRE_GetProcessType());

Do you want to keep this printf?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 22 • 8 years ago

Nope, thanks.

	Eric Rahm [:erahm]
	Comment 23 • 8 years ago

Comment on attachment 8830132 [details] [diff] [review]
part 9, xpcshell test

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

Looks good!

	Pulsebot
	Comment 24 • 8 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fef3862cf602
Move PMemoryReportRequest classes to a separate source file. (bug 1321492 part 1, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/10cb18099ec7
Remove the MemoryReportRequestChild dependency on ContentChild. (bug 1321492 part 2, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/031cc4aa622c
Remove PMemoryReportRequest. (bug 1321492 part 3, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3163300021c
Factor ContentParent methods out of nsMemoryReportingManager. (bug 1321492 part 4, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3914280ae6
Move MaybeFileDesc out of PContent and into an IPDL header. (bug 1321492 part 5, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed89028b70a9
Add memory reporting message support to PGPU. (bug 1321492 part 6, r=rhunt, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d86e5aa6144
Add GPU process support to nsMemoryReportingManager. (bug 1321492 part 7, r=erahm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/51fe2c86f610
Allow controlling the GPU process from xpcshell. (bug 1321492 part 8, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f89d6e49794
Add an xpcshell test for memory reporting in the GPU process. (bug 1321492 part 9, r=erahm)

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fef3862cf602
https://hg.mozilla.org/mozilla-central/rev/10cb18099ec7
https://hg.mozilla.org/mozilla-central/rev/031cc4aa622c
https://hg.mozilla.org/mozilla-central/rev/a3163300021c
https://hg.mozilla.org/mozilla-central/rev/9c3914280ae6
https://hg.mozilla.org/mozilla-central/rev/ed89028b70a9
https://hg.mozilla.org/mozilla-central/rev/1d86e5aa6144
https://hg.mozilla.org/mozilla-central/rev/51fe2c86f610
https://hg.mozilla.org/mozilla-central/rev/6f89d6e49794