Closed Bug 1321907 Opened 7 years ago Closed 7 years ago

Compositor Process is not captured in platform profiles

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Harald, Unassigned)

References

Details

Attachments

(3 files)

Nightly on Windows doesn't show compositing markers anymore in profiles as it. Seems like the process isn't profiled as it was before.
This patch basically copies the code in PContent / ContentParent / ContentChild to PGPU / GPUChild / GPUParent. It compiles but is untested. It probably doesn't work because I'm not redirecting GPUParent::RecvGatherProfile to the main thread.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review102890

::: tools/profiler/public/CrossProcessProfilerController.h:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZ_PROFILER_OBSERVER_H
> +#define MOZ_PROFILER_OBSERVER_H
> +
> +#ifdef MOZ_ENABLE_PROFILER_SPS

I should probably move these #ifdefs out of this file and into moz.build.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review103032

I'm very close to r+, but I want to understand why we've got this intermediate forwarding class - that's not clear to me. Big thanks for centralizing all of this stuff and cleaning it up!

::: dom/plugins/ipc/PluginModuleChild.cpp:2614
(Diff revision 2)
>  }
>  
>  mozilla::ipc::IPCResult
> +PluginModuleChild::RecvPauseProfiler(const bool& aPause)
> +{
> +    if (aPause) {

Whoops - I guess we forgot to support this for the plugin process. Thanks. :)

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:2
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +

Let's get rid of this extra newline.

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:56
(Diff revision 2)
> +{
> +}

Is it worth asserting that we've destroyed by the time that we've gotten here? Or maybe just do the Destroy work here?

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:82
(Diff revision 2)
> +    // profiler-subprocess-gather is the request to capture the profile. We
> +    // need to tell the other process that we're interested in its profile,
> +    // and we tell the gatherer that we've forwarded the request, so that it
> +    // can keep track of the number of pending profiles.

Thanks for tossing in the nice documentation! :)

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:153
(Diff revision 2)
> +  mForwarder = nullptr;
> +
> +  if (mGatherer && !mProfile.IsEmpty()) {
> +    mGatherer->OOPExitProfile(mProfile);
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (obs) {
> +    size_t length = ArrayLength(sObserverTopics);
> +    for (size_t i = 0; i < length; ++i) {
> +      obs->RemoveObserver(this, sObserverTopics[i]);
> +    }
> +  }

Out of curiosity - is there a reason why this isn't in a destructor instead?

::: tools/profiler/public/CrossProcessProfilerController.h:41
(Diff revision 2)
> +class ProfilerIPCForwarder {
> +protected:
> +  friend class CrossProcessProfilerControllerImpl;
> +
> +  virtual bool SendStartProfiler(const ProfilerInitParams& aParams) = 0;
> +  virtual bool SendPauseProfiler(const bool& aPause) = 0;
> +  virtual bool SendStopProfiler() = 0;
> +  virtual bool SendGatherProfile() = 0;
> +};

Out of curiosity, why do we want this intermediate class, instead of (for example) having ContentParent, PluginModuleXParent, etc, implement a class, which the controller gets the reference to? Good ol' polymorphism. Or is there a case I'm missing?

::: tools/profiler/public/CrossProcessProfilerController.h:97
(Diff revision 2)
> +    return mProtocol->SendGatherProfile();
> +  }
> +
> +private:
> +  RefPtr<CrossProcessProfilerControllerImpl> mImpl;
> +  ProtocolT* mProtocol;

Do you feel confident that mProtocol will always exist for the lifetime of CrossProcessProfilerController?
Attachment #8823133 - Flags: review?(mconley)
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review102890

> I should probably move these #ifdefs out of this file and into moz.build.

Yep, good call.
Comment on attachment 8824044 [details]
Bug 1321907 - Hook up the GPU process to the profiler.

https://reviewboard.mozilla.org/r/102566/#review103398

::: gfx/ipc/GPUParent.cpp:366
(Diff revision 1)
>  }
>  
> +mozilla::ipc::IPCResult
> +GPUParent::RecvStartProfiler(const ProfilerInitParams& params)
> +{
> +  nsTArray<const char*> featureArray;

It'd be nice if this could be shared with ContentChild somehow, maybe moved to wherever profiler_start is defined. But if it's a big deal don't worry about it.
Attachment #8824044 - Flags: review?(dvander) → review+
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review103032

> Is it worth asserting that we've destroyed by the time that we've gotten here? Or maybe just do the Destroy work here?

Added an assert.

> Out of curiosity - is there a reason why this isn't in a destructor instead?

The observer service keeps a reference to this object, so its destructor won't be called until it has been removed as an observer.
I've moved this code around a bit, things should be clearer now.

> Out of curiosity, why do we want this intermediate class, instead of (for example) having ContentParent, PluginModuleXParent, etc, implement a class, which the controller gets the reference to? Good ol' polymorphism. Or is there a case I'm missing?

I've made that change. Unfortunately this still requires some indirection, because for example PluginModuleParent is the class that implements this class (which I've now calleld ProfilerControllingProcess), but the implementations of the methods that we want to call are in PPluginModuleParent (the protocol base class). So now PluginModuleParent has to manually forward those calls to PPluginModuleParent.
The template trickery in the previous patch was a way to avoid that - or rather, a way to do the forwarding only once, in generic code. I agree that it wasn't very clear, though. I think what I have now is better.

> Do you feel confident that mProtocol will always exist for the lifetime of CrossProcessProfilerController?

Yes; CrossProcessProfilerController is stored in a UniquePtr in a field of the protocol class, so the protocol will outlive it.
Comment on attachment 8824044 [details]
Bug 1321907 - Hook up the GPU process to the profiler.

https://reviewboard.mozilla.org/r/102566/#review103398

> It'd be nice if this could be shared with ContentChild somehow, maybe moved to wherever profiler_start is defined. But if it's a big deal don't worry about it.

It's not a big deal but I'd rather do that in a follow-up bug.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review122696

I would give this an f+, but MozReview doesn't allow that. You're on the right track but I'd like to see it again. Particularly after you've had a chance to think about the complexity issues we talked about on IRC, such as whether ProfilerObserver is necessary.

::: dom/ipc/ContentParent.cpp:1068
(Diff revision 6)
> +ContentParent::SendStartProfiler(const ProfilerInitParams& aParams)
> +{
> +  if (mSubprocess && mIsAlive) {
> +    return PContentParent::SendStartProfiler(aParams);
> +  }
> +  return true;

I would probably use ?: for this, but others might not. Ditto for the functions below.

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:3
(Diff revision 6)
> +/* 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/. */

As per the style guide, new files should start with modelines:

> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:21
(Diff revision 6)
> +
> +static const char* sObserverTopics[] = {
> +  "profiler-started",
> +  "profiler-stopped",
> +  "profiler-paused",
> +  "profiler-resumed",

The inconsistency in the handling of pause/resume is unfortunate -- here we have separate messages, elsewhere we have a single function with a bool param. Not sure if it's worth the effort to fix, though.

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:30
(Diff revision 6)
> +
> +// ProfilerObserver is a refcounted class that gets registered with the
> +// observer service and just forwards Observe() calls to mController.
> +// The purpose of this indirection is to avoid making
> +// CrossProcessProfilerController a refcounted class, because we'd like to
> +// manage its lifetime through a UniquePtr.

Why do you want to use a UniquePtr? Is the benefit big enough to warrant writing a wrapper class that uses a raw pointer? It doesn't seem so to me, though I may be overlooking something.

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:44
(Diff revision 6)
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +
> +  void Destroy() { mController = nullptr; }
> +private:

Nit: blank line before "private".

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:105
(Diff revision 6)
> +      obs->RemoveObserver(mObserver, sObserverTopics[i]);
> +    }
> +  }
> +
> +  mObserver->Destroy();
> +  mObserver = nullptr;

Is the Destroy() really necessary? When you null out mObserver, ~ProfileObserver() will be called anyway.
Attachment #8823133 - Flags: review?(n.nethercote) → review-
Also: CrossProcessProfilerController's constructor sets mIsProfilerActive to false but also has a local variable profilerActive which is set via IsActive(). Should the IsActive() call instead set mIsProfilerActive?

Alternatively, can we get rid of mIsProfilerActive altogether? profiler_active() is always available, and I like the idea of using that more than having two separate sources of truth.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review124300

Thanks mstange! I'm quite pleased with this refactoring - it's good to see this common stuff get slurped out into its own re-usable class, especially as we seem to be adding more and more process types these days.

njn has some good points above - clearing review request until those are addressed.

::: dom/ipc/ContentParent.cpp:1068
(Diff revision 6)
> +ContentParent::SendStartProfiler(const ProfilerInitParams& aParams)
> +{
> +  if (mSubprocess && mIsAlive) {
> +    return PContentParent::SendStartProfiler(aParams);
> +  }
> +  return true;

Why are we returning `true` here? Haven't we definitely failed to start the profiler if we've reached here? Same question with the other functions below.

::: dom/plugins/ipc/PluginModuleChild.cpp:2722
(Diff revision 6)
> +PluginModuleChild::RecvPauseProfiler(const bool& aPause)
> +{
> +    if (aPause) {
> +        profiler_pause();
> +    } else {
> +        profiler_resume();
> +    }
> +
> +    return IPC_OK();
> +}

Oh good. It was an oversight that the plugin process didn't get this function.
Attachment #8823133 - Flags: review?(mconley)
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review122696

> I would probably use ?: for this, but others might not. Ditto for the functions below.

I don't have an opinion on this. I'll use ?:.

> The inconsistency in the handling of pause/resume is unfortunate -- here we have separate messages, elsewhere we have a single function with a bool param. Not sure if it's worth the effort to fix, though.

Maybe... not in this bug though.

> Why do you want to use a UniquePtr? Is the benefit big enough to warrant writing a wrapper class that uses a raw pointer? It doesn't seem so to me, though I may be overlooking something.

The biggest reason why I wanted to use a UniquePtr was the fact that nulling out a UniquePtr is guaranteed to call the destructor of CPPC. This is important because we do meaningful work during that destructor and it needs to happen at a certain time.

With a RefPtr, you need to justify that you're the last person to hold a reference to the object.

I tried to get rid of ProfilerObserver and make CPPC implement nsIObserver (and nsISupportsWeakReference), but then I ran into problems when I wanted to register the CPPC with the observer service during the CPPC constructor: The observer service wanted to QueryInterface the CPPC object to nsISupportsWeakReference, but since the CPPC constructor was still running, the vtable setup hadn't completed yet, and the call crashed.

I am going to adjust the comment above ProfilerObserver to mention this. I'm also going to change the raw pointer to a reference to make it even clearer that ProfilerObserver is guaranteed to be destroyed before the CPPC goes away.

> Is the Destroy() really necessary? When you null out mObserver, ~ProfileObserver() will be called anyway.

Well, okay. Dropping all pointers to an object that's being destroyed seemed like good form. (As I said above, I don't really like relying on having a destructor run when I null out a RefPtr, because it's something that the compiler can't check and something that's hard to reason about locally; you need additional information to justify it. But in this case that additional information is fairly close by and obvious enough.)
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review124300

> Why are we returning `true` here? Haven't we definitely failed to start the profiler if we've reached here? Same question with the other functions below.

Hmm, well, the callers all ignore these return values. I'm going to make it return void, just to clear up this confusion.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review122696

> I don't have an opinion on this. I'll use ?:.

Now that this has become a void function, I've gone back to the if.
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review124770

This looks great to me! Thanks mstange!
Attachment #8823133 - Flags: review?(mconley) → review+
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review124772

::: tools/profiler/gecko/CrossProcessProfilerController.cpp:30
(Diff revision 7)
> +
> +// ProfilerObserver is a refcounted class that gets registered with the
> +// observer service and just forwards Observe() calls to mController.
> +// This indirection makes the CrossProcessProfilerController API nicer because
> +// it doesn't require a separate Init() method to register with the observer
> +// service, and because not being refcountet allows CPPC to be managed with a

Typo: "refcountet".
Attachment #8823133 - Flags: review?(n.nethercote) → review+
Comment on attachment 8823133 [details]
Bug 1321907 - Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController.

https://reviewboard.mozilla.org/r/101716/#review124776

Mostly ooks good, but I think you overlooked comment 20. r=me with an answer or changes that address it. Thanks.
(In reply to Nicholas Nethercote [:njn] from comment #20)
> Also: CrossProcessProfilerController's constructor sets mIsProfilerActive to
> false but also has a local variable profilerActive which is set via
> IsActive(). Should the IsActive() call instead set mIsProfilerActive?

It doesn't really matter that much because in the "if (profilerActive)" branch we call StartProfiler which sets mIsProfilerActive to true. And this is just moved code.

> Alternatively, can we get rid of mIsProfilerActive altogether?
> profiler_active() is always available, and I like the idea of using that
> more than having two separate sources of truth.

I agree. I'm going to do that in a separate patch, in this bug.
Comment on attachment 8850238 [details]
Bug 1321907 - Remove mIsProfilerActive.

https://reviewboard.mozilla.org/r/122884/#review125170

Thanks!
Attachment #8850238 - Flags: review?(n.nethercote) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/e6086e949850
Move cross process profiler controlling code from ContentParent and PluginModuleParent into a new class called CrossProcessProfilerController. r=mconley,njn
https://hg.mozilla.org/integration/autoland/rev/33e07f746b5e
Remove mIsProfilerActive. r=njn
https://hg.mozilla.org/integration/autoland/rev/e9043c051769
Hook up the GPU process to the profiler. r=dvander
It looks like part 3 doesn't actually work. I'm not seeing a compositor process in my profiles on Windows.
Depends on: 1352237
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: