Migrate to xpcom based EventTracer

RESOLVED FIXED in mozilla33

Status

()

Core
Gecko Profiler
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8436121 [details]
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.
Attachment #8436121 - Flags: review?(ehsan)
(Assignee)

Comment 2

4 years ago
Created attachment 8436392 [details] [diff] [review]
patch

There's still a leak. Maybe I need to revoke the nsRunnable properly?
Assignee: nobody → bgirard
Attachment #8436121 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8436121 - Flags: review?(ehsan)
Attachment #8436392 - Flags: review?(ehsan)

Comment 3

4 years ago
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

4 years ago
About the leak, could it be that ThreadProfile is the one that holds ThreadResponsiveness alive and lead to the runnable member leaking?
(Assignee)

Comment 5

4 years ago
Created attachment 8437289 [details]
abort callstack
(Assignee)

Comment 6

4 years ago
Created attachment 8437310 [details] [diff] [review]
patch v2

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

https://tbpl.mozilla.org/?tree=Try&rev=a75def6432b9
Attachment #8436392 - Attachment is obsolete: true
Attachment #8436392 - Flags: review?(ehsan)
Attachment #8437310 - Flags: review?(ehsan)

Comment 7

4 years ago
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.)
(Assignee)

Comment 8

4 years ago
Created attachment 8438585 [details] [diff] [review]
patch v3

https://tbpl.mozilla.org/?tree=Try&rev=92d4a0dab83d
Attachment #8437310 - Attachment is obsolete: true
Attachment #8437310 - Flags: review?(ehsan)
Attachment #8438585 - Flags: review?(ehsan)

Comment 9

4 years ago
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.  ;-)
Attachment #8438585 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

4 years ago
Filed bug 1024759 to port MOZ_INSTRUMENT_EVENT_LOOP to this system.
(Assignee)

Comment 11

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/14950c172444
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.