963158 - Profiler shouldn't sample sleeping threads multiple times

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Viktor Stanchev [:vikstrous]]

Right now the profiler will sample sleeping threads multiple times. This requires sending signals to the threads being sampled and slows things down. If we just keep track of which threads are sleeping and make sure we just duplicate the previous sample instead of taking a new one, we can reduce the CPU usage of the profiler and the amount of context switching between threads especially when there's a large number of web worker threads.

Comment 1

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep

Comment 2

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep

Oops.. I included my previous patch in this one. Now it's fixed.

Comment 3

[Viktor Stanchev [:vikstrous]]

CPU usage with and without the patch:

Linux 1 web worker
   profiler     off      on
with patch        2%      3%
without patch     3%     15%

Linux 20 web workers
   profiler     off      on
with patch        2%      5%
without patch     2%     30%  

Windows 1 web worker
   profiler    off      on
with patch       1%      1%
without patch    1%      5%  

Windows 20 web workers
   profiler    off      on
with patch      1%       1%       
without patch   1%      11%

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8364455 [details] [diff] [review]
no_resample_on_sleep

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

::: tools/profiler/GeckoProfiler.h
@@ +162,4 @@
>  static inline void profiler_register_thread(const char* name, void* stackTop) {}
>  static inline void profiler_unregister_thread() {}
>  
> +static inline void profiler_sleep_start() {}

We should add documentation here.

::: tools/profiler/ProfileEntry.cpp
@@ +467,5 @@
>  }
>  
> +void ThreadProfile::DuplicateSample() {
> +  // Scan the whole buffer (even unflushed parts)
> +  for (int readPos = mWritePos; readPos != (mReadPos + mEntrySize - 1) % mEntrySize; readPos = (readPos + mEntrySize - 1) % mEntrySize) {

I think you're doing + mEntrySize here because operator% isn't consident for negative operand. This deserve a comment and and possibly a helper function.

::: tools/profiler/ProfileEntry.h
@@ +93,4 @@
>      return aGenID + 2 <= mGeneration;
>    }
>    void* GetStackTop() const { return mStackTop; }
> +  void DuplicateSample();

I think DuplicateLastSample is a more descriptive name.

::: tools/profiler/platform.cpp
@@ +660,5 @@
>          if (!thread_profile) {
>            continue;
>          }
> +        // This makes sure that the sampling thread creates at least one sample for sleeping threads
> +        thread_profile->GetPseudoStack()->forceSleepingSample();

Shouldn't the initial value take care of this? Why does the start case need special handling?

Comment 5

[(no longer active)]

(Just as a historical note, bug 791357 tracks fixing this problem in the general case.)

Comment 6

[Benoit Girard (:BenWa)]

The numbers look good. That means its worth while taking on the complexity of the patch for the savings.

The overhead numbers on linux appears to be high in general. There might be some performance problem there we can look into but that is separate from this issue.

Good work!

Comment 7

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep2

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8364622 [details] [diff] [review]
no_resample_on_sleep2

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

Functions should either be UppercaseStyle for c++ class methods or use_c_function_style instead. We don't use camel case.

:bent can you review us adding two hooks to WorkerPrivate/message_pump. They are fairly trivial. But will let us stop profiling while these threads are idle which is most of the time for web workers.

::: tools/profiler/platform.cpp
@@ +826,5 @@
>  }
>  
> +void mozilla_sampler_sleep_start() {
> +    PseudoStack *stack = tlsPseudoStack.get();
> +    ASSERT(stack != nullptr);

There should probably early return instead.

Make sure that we handle the case of seeing a sleep_end without first seeing a sleep_start (it should be ignored).

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8364622 [details] [diff] [review]
no_resample_on_sleep2

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

::: dom/workers/WorkerPrivate.cpp
@@ +4006,2 @@
>          WaitForWorkerEvents();
> +        profiler_sleep_end();

WaitForWorkerEvents is called in several places, and that function actually does two waits, so I think this isn't the right place to annotate. Let's move them inside WaitForWorkerEvents?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Also, what about instrumenting other nsThreads?

Comment 11

[Viktor Stanchev [:vikstrous]]

Jim, can we hook into the HangMonitor's existing instrumentation to make sure we can trigger our sleep_start and sleep_end functions in every case when a thread is sleeping? It seems like HangMonitor::Suspend and HangMonitor::NotifyActivity are also wrapping most sleeping sections already. Are there cases when they are used but the process is not sleeping?

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Viktor Stanchev [:vikstrous] from comment #11)
> Jim, can we hook into the HangMonitor's existing instrumentation to make
> sure we can trigger our sleep_start and sleep_end functions in every case
> when a thread is sleeping? It seems like HangMonitor::Suspend and
> HangMonitor::NotifyActivity are also wrapping most sleeping sections
> already. Are there cases when they are used but the process is not sleeping?

You'd want BackgroundHangMonitor because HangMonitor is main thread only. But otherwise it should work.

You can try hooking into BackgroundHangMonitor::NotifyActivity and BackgroundHangMonitor::NotifyWait (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/BackgroundHangMonitor.cpp?rev=134244c3d863#476), or the corresponding methods in BackgroundHangThread.

Right now BackgroundHangMonitor is enabled for IPC threads and the main thread, but it'd be great if you can add it to other threads :)

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

You should be able to hit all nsThreads by hooking nsEventQueue::GetEvent I think.

Comment 14

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep4

In this patch I added profiler_sleep_start and profiler_sleep_end to a few more places. Here is the final list:

WorkerPrivate::WaitForWorkerEvents(PRIntervalTime aInterval)
MessagePumpDefault::Run(Delegate* delegate)
MessagePumpLibevent::Run(Delegate* delegate)
PollWrapper(GPollFD *ufds, guint nfsd, gint timeout_) in widget/gtk/nsAppShell.cpp
nsAppShell::ProcessNextNativeEvent(bool mayWait) in widget/windows/nsAppShell.cpp
nsEventQueue::GetEvent(bool mayWait, nsIRunnable **result)

I don't think the OS X main thread loop is among these. I've tested only on Linux and Windows.

For the purpose of the tests I disabled BackgroundHangManager by making its main thread immediately return. It was causing higher CPU usage when it was on at the same time as the profiler sampling all registered threads. This is an issue we have to look into separately.

CPU usage when profiling every thread:

Linux
   profiler     off      on
with patch      2%       4%
without patch   2%       12%

Windows
   profiler    off      on
with patch     0-1%      1%
without patch  0-1%    2-3%

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 8365234 [details] [diff] [review]
no_resample_on_sleep4

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

Leaving it up to :bent to review the event loop instrumentation.

Bent should be check with bsmedberg as well?

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8365234 [details] [diff] [review]
no_resample_on_sleep4

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

I just reviewed dom/workers and ipc/chromium, r=me with these changes:

::: dom/workers/WorkerPrivate.cpp
@@ +4450,5 @@
>    // reporting may proceed.
>    mBlockedForMemoryReporter = true;
> +  // Let the profiler know that we are not going to be doing anything
> +  // interesting
> +  profiler_sleep_start();

I think I would move this after the notify() to just before the first wait().

Also, please add a newline between the previous code and the beginning of your comment.

@@ +4469,5 @@
>  
>    // No need to notify here as the main thread isn't watching for this state.
>    mBlockedForMemoryReporter = false;
> +  // Resume being profiled
> +  profiler_sleep_end();

I think I would put this right after the while loop.

Also, please add a newline between the previous code and the beginning of your comment.

::: widget/windows/nsAppShell.cpp
@@ +233,1 @@
>        WinUtils::WaitForMessage();

Just FYI, it seems to me like you'd want to move this inside WaitForMessage, but I'd ask a windows widget peer to be sure.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8365234 [details] [diff] [review]
no_resample_on_sleep4

Oops, bugzilla fail. We should get bsmedberg to approve nsEventQueue change, and maybe ask someone in widget about those changes.

Comment 18

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep5

I made the changes suggested in the previous comment. Is this the right place in WinUtils::WaitForMessage ?

Comment 19

[Viktor Stanchev [:vikstrous]]

Comment on attachment 8365302 [details] [diff] [review]
no_resample_on_sleep5

Wrong Benjamin?

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8365302 [details] [diff] [review]
no_resample_on_sleep5

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

r=me for the xpcom/ changes.  The widget/windows/ changes look fine to me, but I am not the right person to be asking about those.  roc, can you review the widget/windows/ changes?

How much do we care that tools/profiler/ is now a dependency for all these other things to build/link?  Not very much, I guess?

Comment 21

[Nathan Froyd [:froydnj]]

Pinging bsmedberg for an answer to the last bit of comment 20.

Comment 22

[Benjamin Smedberg]

Cross-linking is not a problem, this is all in libxul and pretty thoroughly cross-linked anyway.

Comment 23

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep6 r=nfroyd

Comment 24

[Viktor Stanchev [:vikstrous]]

Comment on attachment 8367599 [details] [diff] [review]
no_resample_on_sleep6 r=nfroyd

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

In this new patch I used Atomic for the two variables that are shared between the threads being sampled and the sampler thread.

Comment 25

[Viktor Stanchev [:vikstrous]]

On the main thread we have to resample power and memory (to be added in https://bugzilla.mozilla.org/show_bug.cgi?id=964096) instead of copying them forward

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8367599 [details] [diff] [review]
no_resample_on_sleep6 r=nfroyd

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

Shouldn't we be adding sleep_start/sleep_end calls at a lower level, i.e. in Monitor::Wait?

Comment 27

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 8367599 [details] [diff] [review]
> no_resample_on_sleep6 r=nfroyd
> 
> Review of attachment 8367599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we be adding sleep_start/sleep_end calls at a lower level, i.e. in
> Monitor::Wait?

For now I wanted to catch waiting on top level events just to catch the cases that hurt us the most. I've been trying to minimize how much of gecko we instrument.

sleep is probably good to instrument now.

Monitor::Wait I'm a bit affraid that we're going to be adding overhead here so I'd rather have people profiling pay the cost over adding overhead. I could be convinced otherwise.

Comment 28

[Viktor Stanchev [:vikstrous]]

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

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Girard (:BenWa) from comment #27)
> Monitor::Wait I'm a bit affraid that we're going to be adding overhead here
> so I'd rather have people profiling pay the cost over adding overhead. I
> could be convinced otherwise.

How about trying to measure the overhead?

I assume we can make the overhead minimal when the profiler is not running, with some kind of isProfiling() check? If so, instrumenting Monitor::Wait seems a lot better than instrumenting the call sites.

Comment 30

[Benoit Girard (:BenWa)]

Alright, if we're ok with a well predicted branch there it makes my life much easier.

Comment 31

[Viktor Stanchev [:vikstrous]]

If we add profiler_sleep_start() and profiler_sleep_end() calls inside xpcom's monitors we would have to make sure that they are included only when compiling libxul and not any other libraries / executables. I've tried doing that by wrapping the calls in #ifdef MOZ_XUL or MOZILLA_INTERNAL_API but they don't seem to be specific enough. What's the right way to do this?

Comment 32

[Benjamin Smedberg]

If you just want to do this for code that live in libxul, then MOZILLA_INTERNAL_API should be correct. But you don't say what your actual problem is, so it's hard to help more.

Comment 33

[Viktor Stanchev [:vikstrous]]

Attachment: Profiler shouldn't sample sleeping threads multiple times

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

Comment 34

[Viktor Stanchev [:vikstrous]]

Attachment: Profiler shouldn't sample sleeping threads multiple times

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

Comment 35

[Viktor Stanchev [:vikstrous]]

Attachment: Profiler shouldn't sample sleeping threads multiple times

Fixed the OS X issue: https://tbpl.mozilla.org/?tree=Try&rev=512046b222c1

Kicked off another try server test just in case: https://tbpl.mozilla.org/?tree=Try&rev=b0637c7c0977

Comment 36

[Viktor Stanchev [:vikstrous]]

I tried rebasing to see if that fixes the OS X issue... 

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

Comment 37

[Viktor Stanchev [:vikstrous]]

What does this look like to you? Is this a real issue? Also, do I need another review?

Comment 38

[Benoit Girard (:BenWa)]

The one above? Looks like a random failure.

We still need review on the widget changes.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I thought this patch was going to instrument Monitor::Wait, but it doesn't... why not?

Comment 40

[Viktor Stanchev [:vikstrous]]

xpcom/threads/nsEventQueue.cpp wasn't using Monitor, it was using ReentrantMonitor and it seems like ReentrantMonitor is not inheriting form Monitor, so I assumed they are almost the same, but you just didn't notice that's the one we're using in this case. Was I wrong?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, why not instrument both Monitor::Wait and ReentrantMonitor::Wait?

Comment 42

[Viktor Stanchev [:vikstrous]]

Attachment: Profiler shouldn't sample sleeping threads multiple times

Instrumented Monitor::Wait in addition to ReentrantMonitor:Wait

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8392280 [details] [diff] [review]
Profiler shouldn't sample sleeping threads multiple times

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

::: dom/workers/WorkerPrivate.cpp
@@ +4418,5 @@
>    mMemoryReportCondVar.Notify();
>  
> +  // Let the profiler know that we are not going to be doing anything
> +  // interesting
> +  profiler_sleep_start();

We don't need this now, because the Wait handles it, right? Same for other places below.

Comment 44

[Viktor Stanchev [:vikstrous]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Comment on attachment 8392280 [details] [diff] [review]
> Profiler shouldn't sample sleeping threads multiple times
> 
> Review of attachment 8392280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +4418,5 @@
> >    mMemoryReportCondVar.Notify();
> >  
> > +  // Let the profiler know that we are not going to be doing anything
> > +  // interesting
> > +  profiler_sleep_start();
> 
> We don't need this now, because the Wait handles it, right? Same for other
> places below.

In this case it's using a CondVar directly and not a monitor. Should I wrap the CondVar::Wait instead of Monitor::Wait? Monitor::Wait uses it.

In ipc/chromium/src/base/message_pump_default.cc it's a WritableEvent

In ipc/chromium/src/base/message_pump_libevent.cc it's one run of event_base_loop(). Actually, I think that's not the right place. I'll find the specific place where it blocks inside event_base_loop() instead.

In widget/windows/nsAppShell.cpp it's Windows's WaitMessage

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Viktor Stanchev [:vikstrous] from comment #44)
> In this case it's using a CondVar directly and not a monitor. Should I wrap
> the CondVar::Wait instead of Monitor::Wait? Monitor::Wait uses it.

Yes.

> In ipc/chromium/src/base/message_pump_default.cc it's a WritableEvent

OK, that's fine then.

> In ipc/chromium/src/base/message_pump_libevent.cc it's one run of
> event_base_loop(). Actually, I think that's not the right place. I'll find
> the specific place where it blocks inside event_base_loop() instead.

OK thanks.

> In widget/windows/nsAppShell.cpp it's Windows's WaitMessage

OK, that's fine.

Comment 46

[Viktor Stanchev [:vikstrous]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> (In reply to Viktor Stanchev [:vikstrous] from comment #44)
> > In this case it's using a CondVar directly and not a monitor. Should I wrap
> > the CondVar::Wait instead of Monitor::Wait? Monitor::Wait uses it.
> 
> Yes.
> 
> > In ipc/chromium/src/base/message_pump_default.cc it's a WritableEvent
> 
> OK, that's fine then.
> 
> > In ipc/chromium/src/base/message_pump_libevent.cc it's one run of
> > event_base_loop(). Actually, I think that's not the right place. I'll find
> > the specific place where it blocks inside event_base_loop() instead.
> 
> OK thanks.
> 

It looks like libevent doesn't do waiting separately from event loop execution. The API is "run the event loop (with flags, etc)" but there doesn't seem to be a call to say "wait until there's an event, but don't execute it". That's why I put the profiler sleep calls around the event_base_loop function before. However, this wouldn't sample the events on that event loop at all. The stack trace from the relevant thread on linux looks like:

#0  0x00007ffff6ed9cf9 in syscall () from /usr/lib/libc.so.6
#1  0x00007ffff2842525 in epoll_dispatch (base=0x7fffe0009180, tv=<optimized out>) at /home/v/work/gecko-dev/ipc/chromium/src/third_party/libevent/epoll.c:407
#2  0x00007ffff283e7b0 in event_base_loop (base=0x7fffe0009180, flags=<optimized out>) at /home/v/work/gecko-dev/ipc/chromium/src/third_party/libevent/event.c:1607
#3  0x00007ffff28438e7 in base::MessagePumpLibevent::Run (this=0x7fffe0009110, delegate=0x7fffe7afbd50) at /home/v/work/gecko-dev/ipc/chromium/src/base/message_pump_libevent.cc:332
#4  0x00007ffff284f2ad in RunInternal (this=0xfffffffffffffffc) at /home/v/work/gecko-dev/ipc/chromium/src/base/message_loop.cc:226
#5  RunHandler (this=0xfffffffffffffffc) at /home/v/work/gecko-dev/ipc/chromium/src/base/message_loop.cc:219
#6  MessageLoop::Run (this=0xfffffffffffffffc) at /home/v/work/gecko-dev/ipc/chromium/src/base/message_loop.cc:193
#7  0x00007ffff2854c97 in base::Thread::ThreadMain (this=0x4caa80) at /home/v/work/gecko-dev/ipc/chromium/src/base/thread.cc:162
#8  0x00007ffff2843b37 in ThreadFunc (closure=0x8) at /home/v/work/gecko-dev/ipc/chromium/src/base/platform_thread_posix.cc:39
#9  0x00007ffff7bc70a2 in start_thread () from /usr/lib/libpthread.so.0
#10 0x00007ffff6eddd1d in clone () from /usr/lib/libc.so.6

I took this out for now. I don't think it's possible to instrument this properly with libevent, but I'm not an expert on libevent, so I don't know for sure.

> > In widget/windows/nsAppShell.cpp it's Windows's WaitMessage
> 
> OK, that's fine.

Comment 47

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep12

Made the changes specified in the last comment.

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

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8393559 [details] [diff] [review]
no_resample_on_sleep12

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

Looks good to me but please get Benoit's review for the profiler-specific code, which I'm not familiar with.

Comment 49

[Benoit Girard (:BenWa)]

Comment on attachment 8393559 [details] [diff] [review]
no_resample_on_sleep12

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

Look good, let's just deal with the re-entrancy .

::: tools/profiler/GeckoProfiler.h
@@ +171,5 @@
>  static inline void profiler_register_thread(const char* name, void* stackTop) {}
>  static inline void profiler_unregister_thread() {}
>  
> +// These functions tell the profiler that a thread went to sleep so that we can avoid
> +// sampling it while it's sleeping

Ideally this should not be re-entrant. That is if you have two calls to _start back to back then we should detect that via an assert.

If we fundamentally must be re-entrant (and we should know what call chain forces us to do this) then we should document the function as such.

It looks like all use cases of this are within the same function so we should probably expose a RAII helper to make sure that we always have a _end call.

::: xpcom/glue/Monitor.h
@@ +46,5 @@
>      {
> +#ifdef MOZILLA_INTERNAL_API
> +        profiler_sleep_start();
> +#endif //MOZILLA_INTERNAL_API
> +        nsresult result = mCondVar.Wait(interval);

This is a mCondVar which you have already instrument. Is this hunk needed?

Comment 50

[Viktor Stanchev [:vikstrous]]

(In reply to Benoit Girard (:BenWa) from comment #49)
> Comment on attachment 8393559 [details] [diff] [review]
> no_resample_on_sleep12
> 
> Review of attachment 8393559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Look good, let's just deal with the re-entrancy .
> 
> ::: tools/profiler/GeckoProfiler.h
> @@ +171,5 @@
> >  static inline void profiler_register_thread(const char* name, void* stackTop) {}
> >  static inline void profiler_unregister_thread() {}
> >  
> > +// These functions tell the profiler that a thread went to sleep so that we can avoid
> > +// sampling it while it's sleeping
> 
> Ideally this should not be re-entrant. That is if you have two calls to
> _start back to back then we should detect that via an assert.
> 
> If we fundamentally must be re-entrant (and we should know what call chain
> forces us to do this) then we should document the function as such.
> 
> It looks like all use cases of this are within the same function so we
> should probably expose a RAII helper to make sure that we always have a _end
> call.

I added an assertion to make sure it's not started of stopped twice in a row and changed all calls to RAII.

> 
> ::: xpcom/glue/Monitor.h
> @@ +46,5 @@
> >      {
> > +#ifdef MOZILLA_INTERNAL_API
> > +        profiler_sleep_start();
> > +#endif //MOZILLA_INTERNAL_API
> > +        nsresult result = mCondVar.Wait(interval);
> 
> This is a mCondVar which you have already instrument. Is this hunk needed?

Oops. I thought I removed that.

Comment 51

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep13

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

Comment 52

[Benoit Girard (:BenWa)]

Comment on attachment 8396401 [details] [diff] [review]
no_resample_on_sleep13

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

If the assert isn't hit then great.

::: tools/profiler/PseudoStack.h
@@ +477,5 @@
> +
> +  // Call this whenever the current thread sleeps or wakes up
> +  // Calling setSleeping with the same value twice in a row is an error
> +  void setSleeping(int sleeping) {
> +    MOZ_ASSERT((mSleeping && !sleeping) || (!mSleeping && sleeping));

Can this simply be mSleeping != sleeping?

Comment 53

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep14

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

Comment 54

[Viktor Stanchev [:vikstrous]]

(In reply to Benoit Girard (:BenWa) from comment #52)
> Comment on attachment 8396401 [details] [diff] [review]
> no_resample_on_sleep13
> 
> Review of attachment 8396401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the assert isn't hit then great.
> 
> ::: tools/profiler/PseudoStack.h
> @@ +477,5 @@
> > +
> > +  // Call this whenever the current thread sleeps or wakes up
> > +  // Calling setSleeping with the same value twice in a row is an error
> > +  void setSleeping(int sleeping) {
> > +    MOZ_ASSERT((mSleeping && !sleeping) || (!mSleeping && sleeping));
> 
> Can this simply be mSleeping != sleeping?

Oh, oops, yeah. I was thinking of XOR for some reason.

Comment 55

[Viktor Stanchev [:vikstrous]]

Attachment: no_resample_on_sleep15

Comment 56

[Viktor Stanchev [:vikstrous]]

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

Comment 57

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb79a8f6a4a

Comment 58

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9cb79a8f6a4a