1021990 - Migrate to xpcom based EventTracer

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

+++ This bug was initially created as a clone of Bug #1021072 +++

This bug is to switch to an xpcom based EventTracer for simplicity and portability.

Comment 1

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=27137b0a8cf5

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

There's still a leak. Maybe I need to revoke the nsRunnable properly?

Comment 3

[(no longer active)]

Looks like you're hitting <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#549>.

Also, you don't need MOZ_COUNT_CTOR/DTOR for refcounted classes.

Comment 4

[(no longer active)]

About the leak, could it be that ThreadProfile is the one that holds ThreadResponsiveness alive and lead to the runnable member leaking?

Comment 5

[Benoit Girard (:BenWa)]

Attachment: abort callstack

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch v2

Fixed the leak. Still need to understand the queryinterface problem:

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

Comment 7

[(no longer active)]

Maybe we can take a look at it tomorrow together?  I can help you debug it if needed.  (Not enough of my brain is awake right now to do the review anyway.)

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch v3

https://tbpl.mozilla.org/?tree=Try&rev=92d4a0dab83d

Comment 9

[(no longer active)]

Comment on attachment 8438585 [details] [diff] [review]
patch v3

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

r=me with the below fixed.

::: toolkit/xre/nsAppRunner.cpp
@@ +4002,5 @@
>    }
>  
>  #ifdef MOZ_INSTRUMENT_EVENT_LOOP
> +  if (PR_GetEnv("MOZ_INSTRUMENT_EVENT_LOOP")) {
> +    bool logToConsole = true;

Can you please file a follow-up bug to see if we can this stuff unified?

::: tools/profiler/ProfileEntry.cpp
@@ +162,5 @@
>    , mRssMemory(0)
>    , mUssMemory(0)
>  #endif
>  {
>    mEntries = new ProfileEntry[mEntrySize];

MOZ_COUNT_CTOR/DTOR here as well, please.

::: tools/profiler/ProfileEntry.h
@@ +104,5 @@
>    void* GetStackTop() const { return mStackTop; }
>    void DuplicateLastSample();
> +
> +  ThreadInfo* GetThreadInfo() { return mThreadInfo; }
> +  ThreadResponsiveness* GetThreadResponsiveness() { return &mRespInfo; }

Nit: const.

@@ +111,5 @@
>    FRIEND_TEST(ThreadProfile, InsertOneTagWithTinyBuffer);
>    FRIEND_TEST(ThreadProfile, InsertTagsNoWrap);
>    FRIEND_TEST(ThreadProfile, InsertTagsWrap);
>    FRIEND_TEST(ThreadProfile, MemoryMeasure);
> +  ThreadInfo* mThreadInfo;

This is an owning reference, right?  Please document is at such.

::: tools/profiler/SyncProfile.cpp
@@ +14,1 @@
>  {

Please add MOZ_COUNT_CTOR/DTOR here since this object's lifetime seems to be manually managed?

::: tools/profiler/ThreadResponsiveness.cpp
@@ +16,5 @@
> +
> +class CheckResponsivenessTask : public nsRunnable,
> +                                public nsITimerCallback {
> +public:
> +  CheckResponsivenessTask(TimeStamp* aLastTracerTime)

Nit: explicit.

Also, not sure why you're passing in the timestamp by ref.  Please pass it in by value and add an accessor that returns what we have here.

@@ +18,5 @@
> +                                public nsITimerCallback {
> +public:
> +  CheckResponsivenessTask(TimeStamp* aLastTracerTime)
> +    : mLastTracerTime(aLastTracerTime)
> +    , mMonitor("")

Please pass in a name.  The deadlock detector may use the name IIRC.

@@ +31,5 @@
> +    MOZ_COUNT_DTOR(CheckResponsivenessTask);
> +  }
> +
> +  NS_IMETHOD Run() {
> +  {

wut???  :-)

@@ +67,5 @@
> +
> +private:
> +  TimeStamp* mLastTracerTime;
> +  Monitor mMonitor;
> +  nsRefPtr<nsITimer> mTimer;

Nit: The usual convention is to use nsCOMPtr with XPCOM classes.

@@ +80,5 @@
> +{
> +  MOZ_COUNT_CTOR(ThreadResponsiveness);
> +}
> +
> +ThreadResponsiveness::~ThreadResponsiveness() {

Nit: please put the brace on its own line.

::: tools/profiler/ThreadResponsiveness.h
@@ +14,5 @@
> +class CheckResponsivenessTask;
> +
> +class ThreadResponsiveness {
> +public:
> +  ThreadResponsiveness(ThreadProfile *aThreadProfile);

Nit: explicit.

@@ +20,5 @@
> +  ~ThreadResponsiveness();
> +
> +  void Update();
> +
> +  mozilla::TimeDuration GetResp(const mozilla::TimeStamp& now) {

Wouldn't it be better to call this GetUnresponsiveDuration?  Up to you...

@@ +24,5 @@
> +  mozilla::TimeDuration GetResp(const mozilla::TimeStamp& now) {
> +    return now - mLastTracerTime;
> +  }
> +
> +  bool HasData() {

Nit: make these two const please.

::: tools/profiler/platform.h
@@ +40,5 @@
>  #endif
>  
>  #include <stdint.h>
>  #include <math.h>
> +#include "nsThreadUtils.h"

Nit: please use MainThreadUtils.h instead, as it doesn't pull in the entire world.

@@ +423,5 @@
> +
> +  /**
> +   * May be null for the main thread if the profiler was started during startup
> +   */
> +  nsIThread* GetNSThread() { return mNsThread.get(); }

Nit: const.  Also, please call this GetThread.

@@ +432,5 @@
>    PseudoStack* mPseudoStack;
>    PlatformData* mPlatformData;
>    ThreadProfile* mProfile;
>    void* const mStackTop;
> +  nsCOMPtr<nsIThread> mNsThread;

mThread please.  NS stands for Netscape.  We're past those days.  ;-)

Comment 10

[Benoit Girard (:BenWa)]

Filed bug 1024759 to port MOZ_INSTRUMENT_EVENT_LOOP to this system.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> @@ +104,5 @@
> >    void* GetStackTop() const { return mStackTop; }
> >    void DuplicateLastSample();
> > +
> > +  ThreadInfo* GetThreadInfo() { return mThreadInfo; }
> > +  ThreadResponsiveness* GetThreadResponsiveness() { return &mRespInfo; }
> 
> Nit: const.]

The last one can't be const, we return a pointer to a member which will change. The compiler enforces this for ujs.

> 
> @@ +111,5 @@
> >    FRIEND_TEST(ThreadProfile, InsertOneTagWithTinyBuffer);
> >    FRIEND_TEST(ThreadProfile, InsertTagsNoWrap);
> >    FRIEND_TEST(ThreadProfile, InsertTagsWrap);
> >    FRIEND_TEST(ThreadProfile, MemoryMeasure);
> > +  ThreadInfo* mThreadInfo;
> 
> This is an owning reference, right?  Please document is at such.

No it's not. It's a back pointer to the class that owns us.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v4

https://tbpl.mozilla.org/?tree=Try&rev=07b0cd07e9ce

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch v5

https://tbpl.mozilla.org/?tree=Try&rev=45c8ff603829

Comment 14

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14950c172444

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/14950c172444