734691 - Support multiple threads

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

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)]

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)]

Attachment: Part 1: Rename Stack/Profile to imply their thread specific

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 2: Move Stack to ThreadProfile since it's thread specific

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Part 3: Change JSON format to Comment 0

Comment 5

[Benoit Girard (:BenWa)]

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]]

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)]

(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)]

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)]

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)]

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]]

Oh, sure, if sstream has worse performance don't use it. Probably not worth the time to profile it.

Comment 12

[Daniel Holbert [:dholbert]]

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 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16e0066edea6
https://hg.mozilla.org/integration/mozilla-inbound/rev/05b738efb007
https://hg.mozilla.org/integration/mozilla-inbound/rev/34da6373715f

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/16e0066edea6
https://hg.mozilla.org/mozilla-central/rev/05b738efb007
https://hg.mozilla.org/mozilla-central/rev/34da6373715f

Comment 15

[Benoit Girard (:BenWa)]

Attachment: 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]

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/[e-mail]-410d30598e8e

Comment 17

[(no longer active)]

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)]

Attachment: Part 4: Properly implement the sampler registry

Comment 19

[Benoit Girard (:BenWa)]

(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)]

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]

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/[e-mail]-a4d23afd1100

Comment 22

[Benoit Girard (:BenWa)]

Attachment: 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)]

Attachment: 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

[(no longer active)]

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)]

Attachment: Mockup

Comment 26

[(no longer active)]

Comment on attachment 637782 [details]
Mockup

\o/

Comment 27

[Benoit Girard (:BenWa)]

Attachment: 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]]

What is the current state here? This would be super useful for B2G :)

Comment 29

[Benoit Girard (:BenWa)]

This is a high priority but is blocked on bug 779291.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add multiple thread support to Linux sampler

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hmm I just realized the ThreadInfo thing is totally unnecessary...fixing

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

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)]

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) ([e-mail]) (he/him)]

Attachment: 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)]

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) ([e-mail]) (he/him)]

(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)]

(In reply to James Willcox (:snorp) ([e-mail]) 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) ([e-mail]) (he/him)]

Attachment: Add multi-thread support to profiler

Comment 39

[Benoit Girard (:BenWa)]

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) ([e-mail]) (he/him)]

Attachment: Add multi-thread support to profiler

I still hate this patch, but it's a little better now.

Comment 41

[Benoit Girard (:BenWa)]

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) ([e-mail]) (he/him)]

Attachment: Add multi-thread support to profiler

Alright. Rebased and rewritten once again :)

Comment 43

[Benoit Girard (:BenWa)]

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) ([e-mail]) (he/him)]

Attachment: Add multi-thread support to profiler

Comment 45

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 46

[Benoit Girard (:BenWa)]

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)]

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)]

Attachment: Add multi-thread support to profiler. fix win32 error

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

Comment 49

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89e99ecdf29f

Comment 50

[Phil Ringnalda (:philor)]

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)]

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) ([e-mail]) (he/him)]

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]]

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)]

(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]]

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)]

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)]

Attachment: Add support for mac/win32. WIP

Missing MT JS support need to confirm that Linux still works.

Comment 58

[Benoit Girard (:BenWa)]

Attachment: Add support for mac/win32 + JS Support

This builds on top of your patch

Comment 59

[Benoit Girard (:BenWa)]

Attachment: Add support for mac/win32 + JS Support

Comment 60

[Benoit Girard (:BenWa)]

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)]

Attachment: 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][[e-mail]]]

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)]

If this is green I'll push:
https://tbpl.mozilla.org/?tree=Try&rev=91294e59338b

Comment 64

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/025d24d7fee8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab64b048ad04

Comment 65

[Ryan VanderMeulen [:RyanVM]]

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)]

(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 (Exited; not receiving bugmail, old account, do not use)]

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)]

Selected debug + m2:
https://tbpl.mozilla.org/?tree=Try&rev=48e5bd21ce4d

Comment 69

[Benoit Girard (:BenWa)]

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

Comment 70

[Benoit Girard (:BenWa)]

Attachment: Add support for mac/win32 + JS Support

Fixed mutex and ThreadInfo shutdown leak.

Comment 71

[Benoit Girard (:BenWa)]

Let's try this again:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb37a2ae805f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e090321a025c

Comment 72

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7814e2bc3dd

Comment 73

[Benoit Girard (:BenWa)]

This is turning out to be a lot of pain :(
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4787f90e6b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ee168b506dd7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7ef72c2c00

Comment 74

[Benoit Girard (:BenWa)]

Attachment: 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)]

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

Comment 76

[Benoit Girard (:BenWa)]

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

Comment 77

[Benoit Girard (:BenWa)]

The previous run forced mThreads on, this patch does not:
https://tbpl.mozilla.org/?tree=Try&rev=2aab0da7973b

Comment 78

[Benoit Girard (:BenWa)]

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)]

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]]

Still seeing the 24 byte Mutex leaks on OSX mochitest b-c. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92a339cb4bc

Comment 81

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e045934f78be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c321bc4dc63

Comment 82

[Ryan VanderMeulen [:RyanVM]]

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]]

Looks like the leaks are still in failure conditions.
https://tbpl.mozilla.org/php/getParsedLog.php?id=21955031&tree=Mozilla-Inbound

Comment 84

[Benoit Girard (:BenWa)]

(n+1)th times the chance:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb02e3858e1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e44e02f874

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb02e3858e1f
https://hg.mozilla.org/mozilla-central/rev/f2e44e02f874

Be still my heart...

Comment 86

[Benoit Girard (:BenWa)]

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.