Closed Bug 1021990 Opened 8 years ago Closed 8 years ago

Migrate to xpcom based EventTracer

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached file patch (obsolete) —
+++ 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)
Attached patch patch (obsolete) — Splinter Review
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)
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.
About the leak, could it be that ThreadProfile is the one that holds ThreadResponsiveness alive and lead to the runnable member leaking?
Attached file abort callstack
Attached patch patch v2 (obsolete) — Splinter Review
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)
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.)
Attached patch patch v3 (obsolete) — Splinter Review
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 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+
Filed bug 1024759 to port MOZ_INSTRUMENT_EVENT_LOOP to this system.
(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.
Attached patch patch v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=07b0cd07e9ce
Attachment #8438585 - Attachment is obsolete: true
Attachment #8439963 - Flags: review+
Attached patch patch v5Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=45c8ff603829
Attachment #8439963 - Attachment is obsolete: true
Attachment #8440030 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/14950c172444
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.