Last Comment Bug 734691 - Support multiple threads
: Support multiple threads
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla23
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on: 863703 726369 734707 779291 837460 862500 872947
Blocks: 861863 737967 861287 873914
  Show dependency treegraph
 
Reported: 2012-03-10 16:09 PST by Benoit Girard (:BenWa)
Modified: 2013-05-19 20:41 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Rename Stack/Profile to imply their thread specific (12.85 KB, patch)
2012-03-10 20:34 PST, Benoit Girard (:BenWa)
mstange: review+
Details | Diff | Review
Part 2: Move Stack to ThreadProfile since it's thread specific (5.16 KB, patch)
2012-03-10 20:35 PST, Benoit Girard (:BenWa)
mstange: review+
Details | Diff | Review
Part 3: Change JSON format to Comment 0 (3.43 KB, patch)
2012-03-10 20:36 PST, Benoit Girard (:BenWa)
mstange: review+
Details | Diff | Review
Part 4: Properly implement the sampler registry (9.90 KB, patch)
2012-05-09 19:52 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Part 4: Properly implement the sampler registry (9.71 KB, patch)
2012-05-10 17:48 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Part 4: Mac multi-thread support (wip) (19.56 KB, patch)
2012-06-25 22:28 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Part 4: Mac multi-thread support (24.88 KB, patch)
2012-06-27 17:21 PDT, Benoit Girard (:BenWa)
ehsan: review-
Details | Diff | Review
Mockup (213.87 KB, image/png)
2012-06-28 22:09 PDT, Benoit Girard (:BenWa)
no flags Details
Part 4: Mac multi-thread support (Fix rot) (25.04 KB, patch)
2012-07-12 22:56 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Add multiple thread support to Linux sampler (34.72 KB, patch)
2013-02-14 20:08 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Review
Add multiple thread support to Linux sampler (29.73 KB, patch)
2013-03-01 10:18 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review-
Details | Diff | Review
Add multi-thread support to profiler (29.85 KB, patch)
2013-03-07 12:51 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review-
Details | Diff | Review
Add multi-thread support to profiler (32.00 KB, patch)
2013-03-25 07:50 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review+
Details | Diff | Review
Add multi-thread support to profiler (47.71 KB, patch)
2013-03-28 13:23 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review-
Details | Diff | Review
Add multi-thread support to profiler (47.47 KB, patch)
2013-03-28 14:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review+
Details | Diff | Review
Add multi-thread support to profiler. fix win32 error (47.44 KB, text/plain)
2013-03-28 16:52 PDT, Benoit Girard (:BenWa)
no flags Details
Add support for mac/win32. WIP (29.34 KB, patch)
2013-04-04 12:14 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Add support for mac/win32 + JS Support (57.46 KB, patch)
2013-04-04 17:19 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Add support for mac/win32 + JS Support (56.61 KB, patch)
2013-04-04 17:26 PDT, Benoit Girard (:BenWa)
snorp: review+
Details | Diff | Review
Add support for mac/win32 + JS Support. r=snorp (57.48 KB, patch)
2013-04-05 12:10 PDT, Benoit Girard (:BenWa)
bugs: review+
Details | Diff | Review
Add support for mac/win32 + JS Support (57.77 KB, patch)
2013-04-09 20:39 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Review
Add support for mac/win32 + JS Support (58.20 KB, patch)
2013-04-11 17:15 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review

Description Benoit Girard (:BenWa) 2012-03-10 16:09:50 PST
The profiler should support profiling on multiple thread. Here's a rough sketch of how I envision it.

- We hook in register/unregister where we create threads (NSPR/Chromium IPC)
- We modify the SamplerThread to work on a thread list
- When exporting we modify the JSON format to look like:
{ 
  <Profile meta data>,
  threads: [
    {
      name: "Main",
      <meta data such as thread start/end time>
      samples: <samples>
    }
  ], [
    {
      name: "Compositor",
      <meta data such as thread start/end time>
      samples: <samples>
    }
  ]
}
Comment 1 Benoit Girard (:BenWa) 2012-03-10 16:11:37 PST
I've started implementing this. I've been looking for an atomic pointer type set but apparently all we have is PR_AtomicSet which is int32. Do we have anything else? Perhaps I should just use a lock. We only need to set it on register/unregister and on the SamplerThread.
Comment 2 Benoit Girard (:BenWa) 2012-03-10 20:34:06 PST
Created attachment 604718 [details] [diff] [review]
Part 1: Rename Stack/Profile to imply their thread specific
Comment 3 Benoit Girard (:BenWa) 2012-03-10 20:35:06 PST
Created attachment 604719 [details] [diff] [review]
Part 2: Move Stack to ThreadProfile since it's thread specific
Comment 4 Benoit Girard (:BenWa) 2012-03-10 20:36:11 PST
Created attachment 604720 [details] [diff] [review]
Part 3: Change JSON format to Comment 0
Comment 5 Benoit Girard (:BenWa) 2012-03-10 20:37:37 PST
I'll land these 3 patch as soon as their reviewed since they touch a lot of the code and will cause bitrot, the rest will come later.
Comment 6 Markus Stange [:mstange] [away until June 27] 2012-03-11 05:18:11 PDT
Comment on attachment 604720 [details] [diff] [review]
Part 3: Change JSON format to Comment 0

>diff --git a/tools/profiler/TableTicker.cpp b/tools/profiler/TableTicker.cpp

>-          b.DefineProperty(sample, "name", entry.mTagData);
>+          b.DefineProperty(sample, "name", (const char*)entry.mTagData);

Why?

>           frames = b.CreateArray();
>           b.DefineProperty(sample, "frames", frames);
>           b.ArrayPush(samples, sample);
>           break;
>         case 'c':
>         case 'l':
>           {
>             if (sample) {
>               JSObject *frame = b.CreateObject();
>-              b.DefineProperty(frame, "location", entry.mTagData);
>+              char tagBuff[1024];
>+              void* pc = (void*)entry.mTagData;
>+              snprintf(tagBuff, 1024, "%p", pc);
>+              b.DefineProperty(frame, "location", tagBuff);

Would this work? I cringe every time I see fixed buffer lengths.

std::sstream tag;
tag << static_cast<void*>(entry.mTagData);
b.DefineProperty(frame, "location", tag.str().c_str());


So the string-based profile format is now officially deprecated? Have we done any measurements how long JS object construction takes vs. string construction for large profiles?
Comment 7 Benoit Girard (:BenWa) 2012-03-11 07:01:42 PDT
(In reply to Markus Stange from comment #6)
> Comment on attachment 604720 [details] [diff] [review]
> Part 3: Change JSON format to Comment 0
> 
> >diff --git a/tools/profiler/TableTicker.cpp b/tools/profiler/TableTicker.cpp
> 
> >-          b.DefineProperty(sample, "name", entry.mTagData);
> >+          b.DefineProperty(sample, "name", (const char*)entry.mTagData);
> 
> Why?
> 

I just wanted to be explicit which implementation of DefineProperty should be called here.

> >           frames = b.CreateArray();
> >           b.DefineProperty(sample, "frames", frames);
> >           b.ArrayPush(samples, sample);
> >           break;
> >         case 'c':
> >         case 'l':
> >           {
> >             if (sample) {
> >               JSObject *frame = b.CreateObject();
> >-              b.DefineProperty(frame, "location", entry.mTagData);
> >+              char tagBuff[1024];
> >+              void* pc = (void*)entry.mTagData;
> >+              snprintf(tagBuff, 1024, "%p", pc);
> >+              b.DefineProperty(frame, "location", tagBuff);
> 
> Would this work? I cringe every time I see fixed buffer lengths.
> 
> std::sstream tag;
> tag << static_cast<void*>(entry.mTagData);
> b.DefineProperty(frame, "location", tag.str().c_str());
> 

1024 is long enough, assuming we don't compile on a platform with 8000 bits addresses, but your snippet is better right.

> 
> So the string-based profile format is now officially deprecated? Have we
> done any measurements how long JS object construction takes vs. string
> construction for large profiles?

I was thinking that it would still work but wouldn't receive any new features like thread support. And once all the users of the API had upgraded and we know the code performs well enough we can rip it out. I'll measure it today.
Comment 8 Benoit Girard (:BenWa) 2012-03-11 08:05:00 PDT
I'm seeing a consistent x5 increase in time to dump the profile and that's without your patch to bug 733861. However on the flip side we don't have to parse the profile in JS. When sending the profile to the symbol server it will have to be parsed in JSON format to symbolicate which will be significant over head in the server.
Comment 9 Benoit Girard (:BenWa) 2012-03-11 08:20:49 PDT
To be exact I see roughly x4 increase while with your sstream changes I see a x6 increase compared to the text dump.
Comment 10 Benoit Girard (:BenWa) 2012-03-11 08:41:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76566398d36
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5fc6a1133e

Waiting to land Part 3 until we decide if we want to use sstream since it appears to have worse performance.
Comment 11 Markus Stange [:mstange] [away until June 27] 2012-03-11 08:48:33 PDT
Oh, sure, if sstream has worse performance don't use it. Probably not worth the time to profile it.
Comment 12 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-03-11 20:02:19 PDT
csets from comment 10 were just merged to m-c:
  https://hg.mozilla.org/mozilla-central/rev/a76566398d36
  https://hg.mozilla.org/mozilla-central/rev/5f5fc6a1133e
as were their backouts (this was backed out on inbound shortly after it landed there):
  https://hg.mozilla.org/mozilla-central/rev/4db15a238ba4
  https://hg.mozilla.org/mozilla-central/rev/801514b8c35f
Comment 15 Benoit Girard (:BenWa) 2012-05-09 19:52:33 PDT
Created attachment 622610 [details] [diff] [review]
Part 4: Properly implement the sampler registry

Code still needs to be fixed before we insert more then one sampler in the registry. For example as-is if we added more sampler we would spam more then one sampling thread.
Comment 16 Mozilla RelEng Bot 2012-05-09 21:45:18 PDT
Try run for 410d30598e8e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=410d30598e8e
Results (out of 8 total builds):
    success: 2
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-410d30598e8e
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-10 12:26:21 PDT
Comment on attachment 622610 [details] [diff] [review]
Part 4: Properly implement the sampler registry

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

::: tools/profiler/platform-linux.cc
@@ +158,5 @@
> +      LOG("SignalSender usleep error");
> +      ASSERT(result == 0 || errno == EINTR);
> +    }
> +#endif
> +    mozilla::unused << result;

Please just move the declaration inside the #ifdef and get rid of this.

@@ +173,5 @@
> +          Sampler* sampler = list[i];
> +          while (sampler->IsActive()) {
> +            SampleContext(sampler);
> +          }
> +        }

See the comments for the Windows implementation here too.

@@ +259,5 @@
>  
>  
>  void Sampler::Stop() {
>    SetActive(false);
> +  SamplerThread::RemoveActiveSampler(this);

Hmm, why are you not doing this in platform-macos.cc as well?

::: tools/profiler/platform-macos.cc
@@ +213,5 @@
> +          Sampler* sampler = list[i];
> +          while (sampler->IsActive()) {
> +            SampleContext(sampler);
> +          }
> +        }

See the comments on the equivalent Windows code.  We might be at a stage where we can rip some of this out and refactor it into a common base or something (in another bug).

::: tools/profiler/platform-win32.cc
@@ +65,5 @@
>    virtual void Run() {
> +    while (true) {
> +      {
> +        ScopedLock lock(SamplerRegistry::mutex);
> +        std::vector<Sampler*> list = SamplerRegistry::samplerList;

This copies the list.  Is this what you intend to do?

@@ +69,5 @@
> +        std::vector<Sampler*> list = SamplerRegistry::samplerList;
> +        if (list.size() == 0)
> +          break;
> +        for (int i = 0; i < list.size(); i++) {
> +          Sampler* sampler = list[i];

Hmm, since you're using vector, how about using iterators here?

-- or, you can move the IsActive check to the beginning of SampleContext, and use std::for_each (probably for the first time in mozilla code!) \o/
Comment 18 Benoit Girard (:BenWa) 2012-05-10 17:48:24 PDT
Created attachment 622989 [details] [diff] [review]
Part 4: Properly implement the sampler registry
Comment 19 Benoit Girard (:BenWa) 2012-05-10 17:50:35 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> @@ +259,5 @@
> >  
> >  
> >  void Sampler::Stop() {
> >    SetActive(false);
> > +  SamplerThread::RemoveActiveSampler(this);
> 
> Hmm, why are you not doing this in platform-macos.cc as well?

Code is already there, platform-macos.cc already partly support the samplers.

> @@ +65,5 @@
> >    virtual void Run() {
> > +    while (true) {
> > +      {
> > +        ScopedLock lock(SamplerRegistry::mutex);
> > +        std::vector<Sampler*> list = SamplerRegistry::samplerList;
> 
> This copies the list.  Is this what you intend to do?

Changed to const T&

> 
> @@ +69,5 @@
> > +        std::vector<Sampler*> list = SamplerRegistry::samplerList;
> > +        if (list.size() == 0)
> > +          break;
> > +        for (int i = 0; i < list.size(); i++) {
> > +          Sampler* sampler = list[i];
> 
> Hmm, since you're using vector, how about using iterators here?
> 
> -- or, you can move the IsActive check to the beginning of SampleContext,
> and use std::for_each (probably for the first time in mozilla code!) \o/

I find this code more readable. Most people aren't familiar with the for_each interface therefore I see no benefit in using it at the expensive of readability. This code is clear.
Comment 20 Benoit Girard (:BenWa) 2012-05-10 21:37:45 PDT
Comment on attachment 622989 [details] [diff] [review]
Part 4: Properly implement the sampler registry

Got some correctness problem as well as build failure. Let me revise it again first.
Comment 21 Mozilla RelEng Bot 2012-05-10 22:01:20 PDT
Try run for a4d23afd1100 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a4d23afd1100
Results (out of 8 total builds):
    success: 2
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-a4d23afd1100
Comment 22 Benoit Girard (:BenWa) 2012-06-25 22:28:26 PDT
Created attachment 636600 [details] [diff] [review]
Part 4: Mac multi-thread support (wip)

Need to fix a few leaks and perhaps improve the code.

There's no UI support so the extra samples aren't getting displayed.
Comment 23 Benoit Girard (:BenWa) 2012-06-27 17:21:03 PDT
Created attachment 637321 [details] [diff] [review]
Part 4: Mac multi-thread support

It works on top of dynamic labels + js + interwinding! I'm working on the front end changes while I wait for review.
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-28 08:42:18 PDT
Comment on attachment 637321 [details] [diff] [review]
Part 4: Mac multi-thread support

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

::: ipc/chromium/src/base/thread.cc
@@ +136,5 @@
>    message_loop_->PostTask(FROM_HERE, new ThreadQuitTask());
>  }
>  
>  void Thread::ThreadMain() {
> +  SAMPLER_REGISTER_THREAD(name_.c_str());

I don't think we should use the threading library used as a decision point on whether to profile a thread or not.  This should happen in the thread routine for each thread that wants to opt into being profiled.

::: tools/profiler/TableTicker.cpp
@@ +340,5 @@
>    {
>      return mStack;
>    }
> +
> +  int getEntrySize()

Nit: const.

@@ +391,5 @@
> +    for (size_t i = 0; i < mThreadProfileList.size(); i++) {
> +      if (mThreadProfileList[i] != GetPrimaryThreadProfile()) {
> +        delete mThreadProfileList[i];
> +      }
> +    }

This is not great... :/

@@ +567,5 @@
>    b.DefineProperty(profile, "threads", threads);
>  
>    // For now we only have one thread
>    SetPaused(true);
> +  if (mThreadProfileList.size() == 0) {

Nit: empty()

@@ +799,5 @@
> +        sample->threadName,
> +        mPrimaryThreadProfile.getEntrySize(), sample->stack);
> +    }
> +    mThreadProfileList.push_back(*sample->threadProfilePtr);
> +  }

It's not clear to me why you chose threadProfilePtr to be a pointer to a pointer...  Looks to me like you can drop one level of indirection and everything would still work.

@@ +941,5 @@
>  }
>  
> +void mozilla_sampler_register_thread(const char* aName)
> +{
> +  ProfileStack *stack = new ProfileStack();

You're also leaking all ProfileStack objects, it seems.

::: tools/profiler/platform-linux.cc
@@ +51,5 @@
>  }
>  #endif
>  
>  static Sampler* sActiveSampler = NULL;
> +class ThreadProfile;

Nit: not needed, please drop it.

@@ +112,5 @@
> +  ThreadProfile* no_multithread = NULL;
> +  sample->stack = NULL;
> +  sample->threadId = 0;
> +  sample->threadName = "";
> +  sample->threadProfilePtr = &no_multithread;

Hmm, this makes threadProfilePtr point to a stack variable which goes away as soon as this function returns.  This is bad.  Why not make it null?

::: tools/profiler/platform-macos.cc
@@ +158,5 @@
>  void Thread::Join() {
>    pthread_join(data_->thread_, NULL);
>  }
>  
> +class ThreadProfile;

Nit: drop this please.

@@ +182,5 @@
>  
>    thread_act_t profiled_thread() { return profiled_thread_; }
>    pthread_t profiled_pthread() { return profiled_pthread_; }
>  
> +  ThreadProfile** thread_profile_ptr() { return &thread_profile_; }

Nit: const.

@@ +189,5 @@
> +    thread_profile_ = NULL;
> +  }
> +  ProfileStack* profile_stack() { return profile_stack_; }
> +
> +  const char* thread_name() { return name_; }

Nit: Make both const.

@@ +200,5 @@
>    // we also store the pthread because Mach threads have no concept of stack
>    // and we want to be able to get the stack size when we need to unwind the
>    // stack using frame pointers.
>    pthread_t profiled_pthread_;
> +  // Used the store the samples collect against this thread

Nit: collected.

@@ +236,5 @@
> +
> +    const std::vector<Sampler::PlatformData*>& childs = sampler->GetChilds();
> +    for (size_t i = 0; i < childs.size(); i++) {
> +      childs[i]->clear_thread_profile();
> +    }

Hmm, std::vector::operator[] has two versions, one const which returns a std::vector::const_reference, and one non-const which returns a std::vector::reference.  You're calling operator[] on a const vector which should give you a const_reference, which I guess is a `Sampler::PlatformData* const`, and then you call a non-const function for me.  That took me about 10 minutes to figure out. :-)

Also, I'd use the iterator interfaces to iterate over vectors, but YMMV.

@@ +259,5 @@
>    void SampleContext(Sampler* sampler) {
> +    const std::vector<Sampler::PlatformData*>& threads = sampler->GetChilds();
> +    for (size_t i = 0; i < threads.size(); i++) {
> +      SampleContext(sampler, i, threads[i]);
> +    }

Same comment about the vector usage above applies here too.

@@ +365,5 @@
>  {
>    return aData->profiled_pthread();
>  }
> +
> +Mutex* Sampler::threadListMutex_;

Nit: init to null, even though the compiler implicitly does this for you.

@@ +366,5 @@
>    return aData->profiled_pthread();
>  }
> +
> +Mutex* Sampler::threadListMutex_;
> +std::vector<Sampler::PlatformData*> Sampler::threadList_;

glandium will tell you that static objects with constructors are a bad idea.  :-)

@@ +384,5 @@
> +
> +std::vector<Sampler::PlatformData*> Sampler::GetThreadList()
> +{
> +  ScopedLock lock(threadListMutex_);
> +  return threadList_;

Please add a comment saying that lock's destructor gets executed after the vector's copy ctor.

::: tools/profiler/platform-win32.cc
@@ +5,5 @@
>  #include <windows.h>
>  #include "platform.h"
>  #include <process.h>
>  
> +class ThreadProfile;

Nit: drop this please.

@@ +105,5 @@
> +      ThreadProfile* no_multithread = NULL;
> +      sample->stack = NULL;
> +      sample->threadId = 0;
> +      sample->threadName = "";
> +      sample->threadProfilePtr = &no_multithread;

Ditto.

::: tools/profiler/platform.h
@@ +185,5 @@
>    mozilla::TimeStamp timestamp;
>  };
>  
> +class ThreadProfile;
> +class ProfileStack;

Nit: no need to double forward declare.

@@ +225,5 @@
>    class PlatformData;
>  
>    PlatformData* platform_data() { return data_; }
>  
> +  const std::vector<PlatformData*>& GetChilds() { return childs_; }

Nit: make this const please.  Also, GetChildren?  :-)

@@ +239,5 @@
>  #endif
> +
> +  static bool RegisterCurrentThread(const char* aName, ProfileStack* aStack);
> +  // Make a copy of the thread list
> +  static std::vector<PlatformData*> GetThreadList();

Nit: make this const please.

I assume the vector copying here is intentional, given that the implementation of this function locks, right?

@@ +251,2 @@
>    PlatformData* data_;  // Platform specific data.
> +  std::vector<PlatformData*> childs_;

Nit: children_?  :-)

So, storing pointers in vectors like this is not a good idea.  This will make it very hard to manage the lifetime of PlatformData objects, and as far as I can tell, your patch currently doesn't manage their lifetime at all.

The same comment applies to the threadList_ member.

::: tools/profiler/sps_sampler.h
@@ +36,5 @@
>  #define SAMPLER_INIT() mozilla_sampler_init()
>  #define SAMPLER_DEINIT() mozilla_sampler_deinit()
>  #define SAMPLER_START(entries, interval, features, featureCount) mozilla_sampler_start(entries, interval, features, featureCount)
>  #define SAMPLER_STOP() mozilla_sampler_stop()
> +#define SAMPLER_REGISTER_THREAD(name) mozilla_sampler_register_thread(name)

There is currently no way to unregister a thread, which is bad for threads who don't live as long as Gecko does.

And even for permanent threads like the compositor, this may lead into weird stuff (or crashes) to happen during shutdown between the time that those threads die and the profiler thread dies.
Comment 25 Benoit Girard (:BenWa) 2012-06-28 22:09:46 PDT
Created attachment 637782 [details]
Mockup
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-29 10:09:12 PDT
Comment on attachment 637782 [details]
Mockup

\o/
Comment 27 Benoit Girard (:BenWa) 2012-07-12 22:56:51 PDT
Created attachment 641750 [details] [diff] [review]
Part 4: Mac multi-thread support (Fix rot)

Fixed the rot + applied a few review comments. Left the more complex comments pending.
Comment 28 Gregor Wagner [:gwagner] 2013-02-01 12:10:54 PST
What is the current state here? This would be super useful for B2G :)
Comment 29 Benoit Girard (:BenWa) 2013-02-01 12:21:47 PST
This is a high priority but is blocked on bug 779291.
Comment 30 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-02-14 20:08:04 PST
Created attachment 714247 [details] [diff] [review]
Add multiple thread support to Linux sampler
Comment 31 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-02-14 20:10:07 PST
Hmm I just realized the ThreadInfo thing is totally unnecessary...fixing
Comment 32 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-02-14 20:12:32 PST
Nevermind, I was confused by the mac stuff in the original. PlatformData is a little different in the Linux backend...
Comment 33 Benoit Girard (:BenWa) 2013-02-14 20:52:34 PST
Comment on attachment 714247 [details] [diff] [review]
Add multiple thread support to Linux sampler

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

Few more things I want to look at tomorrow.

::: dom/workers/RuntimeService.cpp
@@ +508,5 @@
>        NS_ERROR("Failed to create runtime and context!");
>        return NS_ERROR_FAILURE;
>      }
>  
> +    SAMPLER_REGISTER_THREAD("Worker");

WebWorker

::: tools/profiler/TableTicker.cpp
@@ +160,5 @@
>      , mLastFlushPos(0)
>      , mReadPos(0)
>      , mEntrySize(aEntrySize)
>      , mStack(aStack)
> +    , mThreadName(aName)

It would be nice to include the TID right away. That was it isn't treated as optional in MT profiles.

@@ +419,5 @@
>        readPos = (readPos + incBy) % mEntrySize;
>      }
>    }
>  
> +  ProfileStack* GetStack() const

const ProfileStack*

@@ +1007,2 @@
>  
> +  if (!mJankOnly && !sLastTracerEvent.IsNull() && sample &&

!mJankOnly was removed. Perhaps this slipped in when rebasing.

@@ +1296,5 @@
>      return;
>    }
>  
> +  // FIXME: Main probably isn't right?
> +  ThreadProfile threadProfile("Main", 1000, stack);

"Temporary" would probably be fine.

::: tools/profiler/platform-macos.cc
@@ +365,5 @@
>  }
> +
> +Mutex* Sampler::threadListMutex_ = NULL;
> +// TODO Remove objects with static constructors
> +std::vector<Sampler::PlatformData*> Sampler::threadList_ = NULL;

We should fix this before landing.

@@ +370,5 @@
> +
> +bool
> +Sampler::RegisterCurrentThread(const char* aName, ProfileStack *aStack)
> +{
> +  if (!threadListMutex_) {

It would be cleaner to have profiler_init call a Sampler::Init(). This will get called from profiler_init so it's safe but it's not good code.
Comment 34 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-01 10:18:04 PST
Created attachment 720016 [details] [diff] [review]
Add multiple thread support to Linux sampler

Another patch fixed up a bit. Only implements multi-thread for Linux. Mac and Win32 should work with the one thread as they did before.
Comment 35 Benoit Girard (:BenWa) 2013-03-01 10:48:48 PST
Comment on attachment 720016 [details] [diff] [review]
Add multiple thread support to Linux sampler

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

::: tools/profiler/TableTicker.cpp
@@ +490,5 @@
>      //XXX: It's probably worth splitting the jank profiler out from the regular profiler at some point
>      mJankOnly = hasFeature(aFeatures, aFeatureCount, "jank");
>      mProfileJS = hasFeature(aFeatures, aFeatureCount, "js");
>      mAddLeafAddresses = hasFeature(aFeatures, aFeatureCount, "leaf");
> +    //mPrimaryThreadProfile.addTag(ProfileEntry('m', "Start"));

Do you mean to remove this? Why?

@@ +965,5 @@
> +    *sample->threadProfilePtr = new ThreadProfile(
> +      sample->threadName, mEntrySize, sample->stack,
> +      sample->threadId, sample->isMainThread);
> +
> +    mThreadProfileList.push_back(*sample->threadProfilePtr);

TableTicker::tick runs in a signal handler. push_back can allocate memory. This isn't safe.

@@ +1299,5 @@
>      MOZ_ASSERT(false);
>      return;
>    }
>  
> +  ThreadProfile threadProfile("Gecko", 1000, stack, 0, true);

Nit: Temp or Unwind. This object doesn't persist.

::: tools/profiler/platform-linux.cc
@@ +84,5 @@
>  #include "android-signal-defs.h"
>  #endif
>  
> +static ThreadInfo* sCurrentThreadInfo = NULL;
> +static std::list<ThreadInfo*> sRegisteredThreads;

This code isn't specific to platform-linux. It shouldn't leave here. Perhaps we can create a new file.

@@ +192,5 @@
> +        while (it != sRegisteredThreads.end()) {
> +          sCurrentThreadInfo = *it;
> +          if (tgkill(vm_tgid_, sCurrentThreadInfo->ThreadId(), SIGPROF) < 0) {
> +            // tid was invalid in some way, lets not sample it anymore
> +            it = sRegisteredThreads.erase(it);

I don't like this. Does this happen normally? If not I rather crash, otherwise I rather handle this properly. I can see MT profiling yield some weird results if we just decide to stop sampling a thread. This needs more info at the very least.

@@ +197,5 @@
> +            continue;
> +          }
> +
> +          // Wait for the signal handler to run before moving on to the next one
> +          while (sCurrentThreadInfo)

This means that 'sCurrentThreadInfo' needs to be voliatile.

We said that tgkill blocked everything. Why is this needed?

@@ +353,5 @@
> +  std::list<ThreadInfo*>::iterator it;
> +
> +  pid_t tid;
> +  for (it = sRegisteredThreads.begin(); it != sRegisteredThreads.end(); it++) {
> +    if ((*it)->ThreadId() == tid) {

tid doesn't have a value. I don't understand how this works.

::: tools/profiler/platform-macos.cc
@@ +189,5 @@
>  class SamplerThread : public Thread {
>   public:
>    explicit SamplerThread(int interval)
>        : Thread("SamplerThread"),
> +        interval_(interval), threadProfile_(NULL) {}

Lets clean this up to be
  : Thread...
  , internval_
  , threadProfile_

::: tools/profiler/platform-win32.cc
@@ +68,5 @@
>   public:
>    SamplerThread(int interval, Sampler* sampler)
>        : Thread("SamplerThread"),
>          interval_(interval),
> +        sampler_(sampler), threadProfile_(NULL) {}

same
Comment 36 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-01 11:35:42 PST
(In reply to Benoit Girard (:BenWa) from comment #35)
> Comment on attachment 720016 [details] [diff] [review]
> Add multiple thread support to Linux sampler
> 
> Review of attachment 720016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/TableTicker.cpp
> @@ +490,5 @@
> >      //XXX: It's probably worth splitting the jank profiler out from the regular profiler at some point
> >      mJankOnly = hasFeature(aFeatures, aFeatureCount, "jank");
> >      mProfileJS = hasFeature(aFeatures, aFeatureCount, "js");
> >      mAddLeafAddresses = hasFeature(aFeatures, aFeatureCount, "leaf");
> > +    //mPrimaryThreadProfile.addTag(ProfileEntry('m', "Start"));
> 
> Do you mean to remove this? Why?

Yeah need to remove it I guess. We no longer have a mPrimaryThreadProfile, just individual ones.

> 
> @@ +965,5 @@
> > +    *sample->threadProfilePtr = new ThreadProfile(
> > +      sample->threadName, mEntrySize, sample->stack,
> > +      sample->threadId, sample->isMainThread);
> > +
> > +    mThreadProfileList.push_back(*sample->threadProfilePtr);
> 
> TableTicker::tick runs in a signal handler. push_back can allocate memory.
> This isn't safe.

Hey you wrote that code, not me :)

I thought we had stuff that prevented us from stopping inside the malloc lock?

> 
> @@ +1299,5 @@
> >      MOZ_ASSERT(false);
> >      return;
> >    }
> >  
> > +  ThreadProfile threadProfile("Gecko", 1000, stack, 0, true);
> 
> Nit: Temp or Unwind. This object doesn't persist.

Temp it is

> 
> ::: tools/profiler/platform-linux.cc
> @@ +84,5 @@
> >  #include "android-signal-defs.h"
> >  #endif
> >  
> > +static ThreadInfo* sCurrentThreadInfo = NULL;
> > +static std::list<ThreadInfo*> sRegisteredThreads;
> 
> This code isn't specific to platform-linux. It shouldn't leave here. Perhaps
> we can create a new file.

Yeah it's terrible. We could create platform.cc and put it there.

> 
> @@ +192,5 @@
> > +        while (it != sRegisteredThreads.end()) {
> > +          sCurrentThreadInfo = *it;
> > +          if (tgkill(vm_tgid_, sCurrentThreadInfo->ThreadId(), SIGPROF) < 0) {
> > +            // tid was invalid in some way, lets not sample it anymore
> > +            it = sRegisteredThreads.erase(it);
> 
> I don't like this. Does this happen normally? If not I rather crash,
> otherwise I rather handle this properly. I can see MT profiling yield some
> weird results if we just decide to stop sampling a thread. This needs more
> info at the very least.

Well if a thread terminates without unregistering itself, tgkill() would fail (unless the tid had been reused in a new thread). It won't ever crash.

> 
> @@ +197,5 @@
> > +            continue;
> > +          }
> > +
> > +          // Wait for the signal handler to run before moving on to the next one
> > +          while (sCurrentThreadInfo)
> 
> This means that 'sCurrentThreadInfo' needs to be voliatile.

Are you sure? We're modifying it from our own code, compiler should not be confused at all.

> 
> We said that tgkill blocked everything. Why is this needed?

The signal handler is not necessarily scheduled immediately by the kernel, so we yield until it is finished running.

> 
> @@ +353,5 @@
> > +  std::list<ThreadInfo*>::iterator it;
> > +
> > +  pid_t tid;
> > +  for (it = sRegisteredThreads.begin(); it != sRegisteredThreads.end(); it++) {
> > +    if ((*it)->ThreadId() == tid) {
> 
> tid doesn't have a value. I don't understand how this works.

Oh that is definitely busted. Fixed.

> 
> ::: tools/profiler/platform-macos.cc
> @@ +189,5 @@
> >  class SamplerThread : public Thread {
> >   public:
> >    explicit SamplerThread(int interval)
> >        : Thread("SamplerThread"),
> > +        interval_(interval), threadProfile_(NULL) {}
> 
> Lets clean this up to be
>   : Thread...
>   , internval_
>   , threadProfile_

Done

> 
> ::: tools/profiler/platform-win32.cc
> @@ +68,5 @@
> >   public:
> >    SamplerThread(int interval, Sampler* sampler)
> >        : Thread("SamplerThread"),
> >          interval_(interval),
> > +        sampler_(sampler), threadProfile_(NULL) {}
> 
> same

Done
Comment 37 Benoit Girard (:BenWa) 2013-03-01 13:24:18 PST
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #36)
> (In reply to Benoit Girard (:BenWa) from comment #35)
> > Comment on attachment 720016 [details] [diff] [review]
> > Add multiple thread support to Linux sampler
> > 
> > Review of attachment 720016 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: tools/profiler/TableTicker.cpp
> > @@ +490,5 @@
> > >      //XXX: It's probably worth splitting the jank profiler out from the regular profiler at some point
> > >      mJankOnly = hasFeature(aFeatures, aFeatureCount, "jank");
> > >      mProfileJS = hasFeature(aFeatures, aFeatureCount, "js");
> > >      mAddLeafAddresses = hasFeature(aFeatures, aFeatureCount, "leaf");
> > > +    //mPrimaryThreadProfile.addTag(ProfileEntry('m', "Start"));
> > 
> > Do you mean to remove this? Why?
> 
> Yeah need to remove it I guess. We no longer have a mPrimaryThreadProfile,
> just individual ones.

I introduce this to tell if the circular buffer had wrapped. I don't even know if it works properly. I can fix this up later.

> 
> > 
> > @@ +965,5 @@
> > > +    *sample->threadProfilePtr = new ThreadProfile(
> > > +      sample->threadName, mEntrySize, sample->stack,
> > > +      sample->threadId, sample->isMainThread);
> > > +
> > > +    mThreadProfileList.push_back(*sample->threadProfilePtr);
> > 
> > TableTicker::tick runs in a signal handler. push_back can allocate memory.
> > This isn't safe.
> 
> Hey you wrote that code, not me :)

Yes, but I didn't land the code either :)

> 
> I thought we had stuff that prevented us from stopping inside the malloc
> lock?
> 

You mean abort if we malloc in a sighandler? Not that I know of.

> > 
> > @@ +1299,5 @@
> > >      MOZ_ASSERT(false);
> > >      return;
> > >    }
> > >  
> > > +  ThreadProfile threadProfile("Gecko", 1000, stack, 0, true);
> > 
> > Nit: Temp or Unwind. This object doesn't persist.
> 
> Temp it is
> 
> > 
> > ::: tools/profiler/platform-linux.cc
> > @@ +84,5 @@
> > >  #include "android-signal-defs.h"
> > >  #endif
> > >  
> > > +static ThreadInfo* sCurrentThreadInfo = NULL;
> > > +static std::list<ThreadInfo*> sRegisteredThreads;
> > 
> > This code isn't specific to platform-linux. It shouldn't leave here. Perhaps
> > we can create a new file.
> 
> Yeah it's terrible. We could create platform.cc and put it there.

sounds good
> 
> > 
> > @@ +192,5 @@
> > > +        while (it != sRegisteredThreads.end()) {
> > > +          sCurrentThreadInfo = *it;
> > > +          if (tgkill(vm_tgid_, sCurrentThreadInfo->ThreadId(), SIGPROF) < 0) {
> > > +            // tid was invalid in some way, lets not sample it anymore
> > > +            it = sRegisteredThreads.erase(it);
> > 
> > I don't like this. Does this happen normally? If not I rather crash,
> > otherwise I rather handle this properly. I can see MT profiling yield some
> > weird results if we just decide to stop sampling a thread. This needs more
> > info at the very least.
> 
> Well if a thread terminates without unregistering itself, tgkill() would
> fail (unless the tid had been reused in a new thread). It won't ever crash.

Ohh ok, let's add a comment then. It should be safe to printf here so we can emit a warning. Or we could crash and force people to unregister.

> 
> > 
> > @@ +197,5 @@
> > > +            continue;
> > > +          }
> > > +
> > > +          // Wait for the signal handler to run before moving on to the next one
> > > +          while (sCurrentThreadInfo)
> > 
> > This means that 'sCurrentThreadInfo' needs to be voliatile.
> 
> Are you sure? We're modifying it from our own code, compiler should not be
> confused at all.

I though it was required because the compiler may move it to a register in which case it would never read back the value.

> 
> > 
> > We said that tgkill blocked everything. Why is this needed?
> 
> The signal handler is not necessarily scheduled immediately by the kernel,
> so we yield until it is finished running.
> 

Ahh ok, comment please.

> > 
> > @@ +353,5 @@
> > > +  std::list<ThreadInfo*>::iterator it;
> > > +
> > > +  pid_t tid;
> > > +  for (it = sRegisteredThreads.begin(); it != sRegisteredThreads.end(); it++) {
> > > +    if ((*it)->ThreadId() == tid) {
> > 
> > tid doesn't have a value. I don't understand how this works.
> 
> Oh that is definitely busted. Fixed.
> 
> > 
> > ::: tools/profiler/platform-macos.cc
> > @@ +189,5 @@
> > >  class SamplerThread : public Thread {
> > >   public:
> > >    explicit SamplerThread(int interval)
> > >        : Thread("SamplerThread"),
> > > +        interval_(interval), threadProfile_(NULL) {}
> > 
> > Lets clean this up to be
> >   : Thread...
> >   , internval_
> >   , threadProfile_
> 
> Done
> 
> > 
> > ::: tools/profiler/platform-win32.cc
> > @@ +68,5 @@
> > >   public:
> > >    SamplerThread(int interval, Sampler* sampler)
> > >        : Thread("SamplerThread"),
> > >          interval_(interval),
> > > +        sampler_(sampler), threadProfile_(NULL) {}
> > 
> > same
> 
> Done
Comment 38 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-07 12:51:47 PST
Created attachment 722444 [details] [diff] [review]
Add multi-thread support to profiler
Comment 39 Benoit Girard (:BenWa) 2013-03-07 16:24:35 PST
Comment on attachment 722444 [details] [diff] [review]
Add multi-thread support to profiler

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

r- because allocations can cause delocks when registering new threads.

I think we need to move these allocations to either when the profiler starts. Or if the profiler is running we will need to grab locks, find the values for the ThreadProfile and allocate it. We could also ignore adding threads while profiling but this is a bit of a pain if you want to profile workers for example.

::: tools/profiler/TableTicker.cpp
@@ +959,5 @@
>   * to know are associated with different events */
>  
>  void TableTicker::Tick(TickSample* sample)
>  {
> +  if (!*sample->threadProfilePtr) {

We shouldn't allow ::Tick to be called with NULL. Sorry this comes from my patch but it's terribly wrong. It's a harder problem on MacOSX anyways. We can do linux without this and I'll try harder to find a solution for OSX. This will fix the allocation problems below:

@@ +960,5 @@
>  
>  void TableTicker::Tick(TickSample* sample)
>  {
> +  if (!*sample->threadProfilePtr) {
> +    *sample->threadProfilePtr = new ThreadProfile(

new will allocate memory. This isn't safe to run within ::Tick

@@ +966,5 @@
> +      sample->threadId, sample->isMainThread);
> +
> +    (*sample->threadProfilePtr)->addTag(ProfileEntry('m', "Start"));
> +
> +    mThreadProfileList.push_back(*sample->threadProfilePtr);

push_back can also allocate memory.

@@ +1128,5 @@
>  
> +void mozilla_sampler_register_thread(const char* aName)
> +{
> +  ProfileStack *stack = new ProfileStack();
> +  tlsStack.set(stack);

You should check 'if (!stack_key_initialized)' before this.
Comment 40 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-25 07:50:25 PDT
Created attachment 728979 [details] [diff] [review]
Add multi-thread support to profiler

I still hate this patch, but it's a little better now.
Comment 41 Benoit Girard (:BenWa) 2013-03-25 08:39:49 PDT
Comment on attachment 728979 [details] [diff] [review]
Add multi-thread support to profiler

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

This looks much better. I'd like for this to still wait for bug 851748+bug 851611 to land because this will bitrot about 300KB of cleanup patches when rebasing this is much simpler. I promise to land them ASAP.

::: tools/profiler/platform-linux.cc
@@ +193,5 @@
> +          for (uint32_t i = 0; i < sRegisteredThreads.size(); i++) {
> +            sCurrentThreadInfo = sRegisteredThreads[i];
> +            if (tgkill(vm_tgid_, sCurrentThreadInfo->ThreadId(), SIGPROF) != 0) {
> +              printf_stderr("profiler failed to signal tid=%d\n", sCurrentThreadInfo->ThreadId());
> +  #ifdef DEBUG

no indent

::: tools/profiler/sps_sampler.h
@@ +68,5 @@
>  
> +#define SAMPLER_REGISTER_THREAD(name) \
> +  do { \
> +    if (!sps_version2()) mozilla_sampler_register_thread1(name); \
> +    else mozilla_sampler_register_thread2(name); \

This stuff is fixed by the refactoring. It will be fixed by the rebase.
Comment 42 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-28 13:23:43 PDT
Created attachment 730842 [details] [diff] [review]
Add multi-thread support to profiler

Alright. Rebased and rewritten once again :)
Comment 43 Benoit Girard (:BenWa) 2013-03-28 13:36:33 PDT
Comment on attachment 730842 [details] [diff] [review]
Add multi-thread support to profiler

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

Just missing the shutdown case.

::: dom/workers/RuntimeService.cpp
@@ +513,5 @@
>        NS_ERROR("Failed to create runtime and context!");
>        return NS_ERROR_FAILURE;
>      }
>  
> +    PROFILE_REGISTER_THREAD("WebWorker");

lowercase

@@ +543,5 @@
>  
>      JS_DestroyRuntime(rt);
>  
>      workerPrivate->ScheduleDeletion(false);
> +    PROFILE_UNREGISTER_THREAD();

lowercase

::: tools/profiler/GeckoProfiler.h
@@ +75,5 @@
>  // Main thread specilization to avoid TLS lookup for performance critical use.
>  #define PROFILER_MAIN_THREAD_LABEL(name_space, info) do {} while (0)
>  #define PROFILER_MAIN_THREAD_LABEL_PRINTF(name_space, info, format, ...) do {} while (0)
>  
> +static inline void PROFILE_REGISTER_THREAD(const char* name) {}

lowercase

::: tools/profiler/GeckoProfilerImpl.h
@@ +165,5 @@
>  #define PROFILER_MAIN_THREAD_LABEL(name_space, info)  MOZ_ASSERT(NS_IsMainThread(), "This can only be called on the main thread"); mozilla::SamplerStackFrameRAII SAMPLER_APPEND_LINE_NUMBER(sampler_raii)(name_space "::" info, __LINE__)
>  #define PROFILER_MAIN_THREAD_LABEL_PRINTF(name_space, info, ...)  MOZ_ASSERT(NS_IsMainThread(), "This can only be called on the main thread"); mozilla::SamplerStackFramePrintfRAII SAMPLER_APPEND_LINE_NUMBER(sampler_raii)(name_space "::" info, __LINE__, __VA_ARGS__)
>  #define PROFILER_MAIN_THREAD_MARKER(info)  MOZ_ASSERT(NS_IsMainThread(), "This can only be called on the main thread"); mozilla_sampler_add_marker(info)
>  
> +static inline void PROFILE_REGISTER_THREAD(const char* name) { profiler_register_thread(name); }

lowercase

::: tools/profiler/platform-linux.cc
@@ +184,5 @@
> +        std::vector<ThreadInfo*> threads = GetRegisteredThreads();
> +
> +        for (uint32_t i = 0; i < threads.size(); i++) {
> +          ThreadInfo* info = threads[i];
> +          sCurrentThreadProfile = info->Profile();

// Use sCurrentThreadProfile to pass the ThreadProfile we're profiling to the signal handler.

@@ +335,5 @@
> +  int id = gettid();
> +
> +  for (uint32_t i = 0; i < sRegisteredThreads.size(); i++) {
> +    if (sRegisteredThreads[i]->ThreadId() == id) {
> +      sRegisteredThreads.erase(sRegisteredThreads.begin() + i);

Needs a delete. These also need to be cleaned up on _shutdown in platform.cpp
Comment 44 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-28 14:11:46 PDT
Created attachment 730874 [details] [diff] [review]
Add multi-thread support to profiler
Comment 45 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-28 14:15:00 PDT
https://tbpl.mozilla.org/?tree=Try&rev=79935acc4f65
Comment 46 Benoit Girard (:BenWa) 2013-03-28 14:18:48 PDT
Comment on attachment 730874 [details] [diff] [review]
Add multi-thread support to profiler

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

Finally. Good job snorp!
Comment 47 Benoit Girard (:BenWa) 2013-03-28 16:45:51 PDT
Comment on attachment 730874 [details] [diff] [review]
Add multi-thread support to profiler

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

::: tools/profiler/platform.cpp
@@ +35,5 @@
>  /* used to keep track of the last event that we sampled during */
>  unsigned int sLastSampledEventGeneration = 0;
>  
>  /* a counter that's incremented everytime we get responsiveness event
> + * note: it might also be worth trackplaing everytime we go around

typo
Comment 48 Benoit Girard (:BenWa) 2013-03-28 16:52:49 PDT
Created attachment 730974 [details]
Add multi-thread support to profiler. fix win32 error

https://tbpl.mozilla.org/?tree=Try&rev=d88e11f5ec58
Comment 50 Phil Ringnalda (:philor) 2013-03-28 21:01:51 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/53b183fe1d62 - probably be a good idea to include debug, and at least one mochitest/reftest/crashtest in that try push, to see that you're leaking one Mutex in every suite.
Comment 51 Phil Ringnalda (:philor) 2013-03-28 21:26:12 PDT
And a Win8 opt xpcshell timeout in devtools\debugger\tests\unit\test_profiler_actor.js, dunno whether that's you-profiler or different-profiler, but at least by blaming you I don't have to file it :)
Comment 52 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-03-29 12:47:45 PDT
Repushed after fixing the leak. https://hg.mozilla.org/integration/mozilla-inbound/rev/7112a6c1efee

I don't think the xpcshell timeout was me, though.

https://tbpl.mozilla.org/?tree=Try&rev=db8a7fe920b6
Comment 53 Ryan VanderMeulen [:RyanVM] 2013-03-29 17:22:08 PDT
Yep, the Win8 xpcshell timeouts were yours. Backed out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a619c7ea5e75

Also, you had OSX 10.8 mochitest-2 24 byte Mutex leaks on a semi-frequent basis.
Comment 54 Benoit Girard (:BenWa) 2013-04-03 10:11:26 PDT
(In reply to Ryan VanderMeulen [:RyanVM] from comment #53)
> Yep, the Win8 xpcshell timeouts were yours. Backed out again.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a619c7ea5e75

I don't see the win8 xpcshell timeouts on either his try push or the inbound push:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7112a6c1efee

Did you see it on a subsequent test run?

> 
> Also, you had OSX 10.8 mochitest-2 24 byte Mutex leaks on a semi-frequent
> basis.

got a fix for that.
Comment 55 Ryan VanderMeulen [:RyanVM] 2013-04-03 10:32:27 PDT
Was nearly a week ago at this point, but my recollection is that yes, they were occurring semi-frequently on subsequent pushes and went away post-backout.
Comment 56 Benoit Girard (:BenWa) 2013-04-03 16:00:02 PDT
Work in progress to port to mac+win32. Checking to make sure it's still build on linux while this is happening:
https://tbpl.mozilla.org/?tree=Try&rev=1116407f67b6
Comment 57 Benoit Girard (:BenWa) 2013-04-04 12:14:17 PDT
Created attachment 733484 [details] [diff] [review]
Add support for mac/win32. WIP

Missing MT JS support need to confirm that Linux still works.
Comment 58 Benoit Girard (:BenWa) 2013-04-04 17:19:02 PDT
Created attachment 733649 [details] [diff] [review]
Add support for mac/win32 + JS Support

This builds on top of your patch
Comment 59 Benoit Girard (:BenWa) 2013-04-04 17:26:19 PDT
Created attachment 733657 [details] [diff] [review]
Add support for mac/win32 + JS Support
Comment 60 Benoit Girard (:BenWa) 2013-04-05 07:09:34 PDT
Comment on attachment 733657 [details] [diff] [review]
Add support for mac/win32 + JS Support

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

Smaug can you review the first two dom/* changes? The profiler needs to register the js operation callback to turn on the profiler if it's pending as directed by the JS team. Since there's only one operation callback per JSRuntime I need to hook into the existing operation callbacks.
Comment 61 Benoit Girard (:BenWa) 2013-04-05 12:10:24 PDT
Created attachment 733993 [details] [diff] [review]
Add support for mac/win32 + JS Support. r=snorp

Minor compilation fixes:
https://tbpl.mozilla.org/?tree=Try&rev=47750a9eba3e
Comment 62 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-04-06 16:30:17 PDT
Comment on attachment 733993 [details] [diff] [review]
Add support for mac/win32 + JS Support. r=snorp

PseudoStack *stack -> PseudoStack* stack

if (expr) {
  stmt;
}

Call profiler_js_operation_callback() only in the workers, per IRC discussion.
Comment 63 Benoit Girard (:BenWa) 2013-04-08 11:30:05 PDT
If this is green I'll push:
https://tbpl.mozilla.org/?tree=Try&rev=91294e59338b
Comment 65 Ryan VanderMeulen [:RyanVM] 2013-04-08 19:17:26 PDT
Hmm, this lands and the 24 byte mutex leaks re-appear. I'm not a big believer in coincidence, so out this goes again. On the bright side, I'm not seeing any xpcshell hangs yet.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1887b25c95

https://tbpl.mozilla.org/php/getParsedLog.php?id=21570517&tree=Mozilla-Inbound

18:39:14  WARNING -  TEST-UNEXPECTED-FAIL | Shutdown | application timed out after 330 seconds with no output
18:39:20  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 24 bytes leaked (Mutex)
Comment 66 Benoit Girard (:BenWa) 2013-04-08 19:22:32 PDT
(In reply to Ryan VanderMeulen [:RyanVM] from comment #65)
> Hmm, this lands and the 24 byte mutex leaks re-appear. I'm not a big
> believer in coincidence, so out this goes again. On the bright side, I'm not
> seeing any xpcshell hangs yet.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1887b25c95
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21570517&tree=Mozilla-
> Inbound
> 
> 18:39:14  WARNING -  TEST-UNEXPECTED-FAIL | Shutdown | application timed out
> after 330 seconds with no output
> 18:39:20  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 24 bytes leaked
> (Mutex)

Yea that's the right call sorry. I didn't see these in my try run :(
Comment 67 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-09 06:00:39 PDT
This also appears to have caused some Talos regressions.

<Regression> Mozilla-Inbound - Trace Malloc Leaks - WINNT 5.2 - 99.3%
etc.
Comment 68 Benoit Girard (:BenWa) 2013-04-09 12:42:16 PDT
Selected debug + m2:
https://tbpl.mozilla.org/?tree=Try&rev=48e5bd21ce4d
Comment 69 Benoit Girard (:BenWa) 2013-04-09 13:39:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d9cd16391864
Comment 70 Benoit Girard (:BenWa) 2013-04-09 20:39:25 PDT
Created attachment 735577 [details] [diff] [review]
Add support for mac/win32 + JS Support

Fixed mutex and ThreadInfo shutdown leak.
Comment 74 Benoit Girard (:BenWa) 2013-04-11 17:15:04 PDT
Created attachment 736578 [details] [diff] [review]
Add support for mac/win32 + JS Support

Fixed the thread de-registration locking.
https://tbpl.mozilla.org/?tree=Try&rev=8f907600b7f4
Comment 75 Benoit Girard (:BenWa) 2013-04-14 16:47:11 PDT
https://tbpl.mozilla.org/?tree=Try&rev=23c84d983fea
Comment 76 Benoit Girard (:BenWa) 2013-04-15 07:53:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=34bc919296bc
Comment 77 Benoit Girard (:BenWa) 2013-04-15 12:07:39 PDT
The previous run forced mThreads on, this patch does not:
https://tbpl.mozilla.org/?tree=Try&rev=2aab0da7973b
Comment 78 Benoit Girard (:BenWa) 2013-04-15 22:18:12 PDT
My qparent was bad causing me a lot of unrelated failures :( Trying again:
https://tbpl.mozilla.org/?tree=Try&rev=a39c1c34b1ba
Comment 79 Benoit Girard (:BenWa) 2013-04-16 07:39:38 PDT
I got a full pass this time with and without forcing on the feature:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2fbf7b0372
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba88d9730af6
Comment 80 Ryan VanderMeulen [:RyanVM] 2013-04-16 10:47:09 PDT
Still seeing the 24 byte Mutex leaks on OSX mochitest b-c. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92a339cb4bc
Comment 82 Ryan VanderMeulen [:RyanVM] 2013-04-18 05:16:14 PDT
Still leaking :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5d5fccbc11

https://tbpl.mozilla.org/php/getParsedLog.php?id=21947004&tree=Mozilla-Inbound

00:42:03  WARNING -  TEST-UNEXPECTED-FAIL | /tests/parser/htmlparser/tests/mochitest/test_bug599584.html | Exited with code -20 during test run
00:42:03     INFO -  INFO | automation.py | Application ran for: 0:04:19.542989
00:42:03     INFO -  INFO | zombiecheck | Reading PID log: /var/folders/s1/zw9hr_nx645fs_fbl2vmsv_h00000w/T/tmpF4MKxUpidlog
00:42:03     INFO -  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, plugin process 900
00:42:03     INFO -       |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
00:42:03     INFO -                                                Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
00:42:03     INFO -     0 TOTAL                                          27       24     1148        1 (   36.61 +/-    25.17)      553        0 (   62.00 +/-    57.15)
00:42:03     INFO -     5 Mutex                                          24       24       36        1 (    4.73 +/-     2.50)        0        0 (    0.00 +/-     0.00)
00:42:03     INFO -  nsTraceRefcntImpl::DumpStatistics: 20 entries
00:42:03     INFO -  TEST-INFO | leakcheck | leaked 1 Mutex (24 bytes)
00:42:03  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 24 bytes leaked (Mutex)
00:42:03     INFO -  INFO | runtests.py | Running tests: end.
00:42:03    ERROR - Return code: 236
Comment 83 Ryan VanderMeulen [:RyanVM] 2013-04-18 05:58:43 PDT
Looks like the leaks are still in failure conditions.
https://tbpl.mozilla.org/php/getParsedLog.php?id=21955031&tree=Mozilla-Inbound
Comment 86 Benoit Girard (:BenWa) 2013-04-18 19:15:39 PDT
This reports a leak using ASAN build. This leak is caused by the pseudostack which we maintain after the profiler has shutdown. The reason we do this is because we don't have to check if the profiler is initialized to push/pop our labels which we do in release builds. We've already discussed this at length during the initial review for the profiler. The leak increases with this because we keep those structure per thread.

Note You need to log in before you can comment on or make changes to this bug.