Closed
Bug 963158
Opened 11 years ago
Closed 11 years ago
Profiler shouldn't sample sleeping threads multiple times
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: vikstrous, Assigned: vikstrous)
References
Details
Attachments
(1 file, 13 obsolete files)
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → vstanchev
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8364453 -
Flags: review?(bgirard)
Assignee | ||
Comment 2•11 years ago
|
||
Oops.. I included my previous patch in this one. Now it's fixed.
Attachment #8364453 -
Attachment is obsolete: true
Attachment #8364453 -
Flags: review?(bgirard)
Attachment #8364455 -
Flags: review?(bgirard)
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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?
Attachment #8364455 -
Flags: review?(bgirard) → review-
Comment 5•11 years ago
|
||
(Just as a historical note, bug 791357 tracks fixing this problem in the general case.)
Comment 6•11 years ago
|
||
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!
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8364455 -
Attachment is obsolete: true
Attachment #8364622 -
Flags: review?(bgirard)
Comment 8•11 years ago
|
||
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).
Attachment #8364622 -
Flags: review?(bgirard)
Attachment #8364622 -
Flags: review?(bent.mozilla)
Attachment #8364622 -
Flags: review+
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?
Attachment #8364622 -
Flags: review?(bent.mozilla) → review-
Also, what about instrumenting other nsThreads?
Assignee | ||
Comment 11•11 years ago
|
||
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?
Flags: needinfo?(nchen)
Comment 12•11 years ago
|
||
(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 :)
Flags: needinfo?(nchen)
You should be able to hit all nsThreads by hooking nsEventQueue::GetEvent I think.
Assignee | ||
Comment 14•11 years ago
|
||
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%
Attachment #8364622 -
Attachment is obsolete: true
Attachment #8365234 -
Flags: review?(bgirard)
Assignee | ||
Updated•11 years ago
|
Attachment #8365234 -
Flags: review?(bent.mozilla)
Comment 15•11 years ago
|
||
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?
Attachment #8365234 -
Flags: review?(bgirard) → review+
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.
Attachment #8365234 -
Flags: review?(bgirard)
Attachment #8365234 -
Flags: review?(bent.mozilla)
Attachment #8365234 -
Flags: review+
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.
Attachment #8365234 -
Flags: review?(bgirard) → review?(benjamin)
Assignee | ||
Comment 18•11 years ago
|
||
I made the changes suggested in the previous comment. Is this the right place in WinUtils::WaitForMessage ?
Attachment #8365234 -
Attachment is obsolete: true
Attachment #8365234 -
Flags: review?(benjamin)
Attachment #8365302 -
Flags: review?(benjamin)
Flags: needinfo?(bent.mozilla)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8365302 [details] [diff] [review]
no_resample_on_sleep5
Wrong Benjamin?
Attachment #8365302 -
Flags: review?(benjamin) → review?(benjamin)
Updated•11 years ago
|
Flags: needinfo?(bent.mozilla)
Updated•11 years ago
|
Attachment #8365302 -
Flags: review?(benjamin) → review?(nfroyd)
Comment 20•11 years ago
|
||
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?
Attachment #8365302 -
Flags: review?(roc)
Attachment #8365302 -
Flags: review?(nfroyd)
Attachment #8365302 -
Flags: review+
Comment 21•11 years ago
|
||
Pinging bsmedberg for an answer to the last bit of comment 20.
Flags: needinfo?(benjamin)
Comment 22•11 years ago
|
||
Cross-linking is not a problem, this is all in libxul and pretty thoroughly cross-linked anyway.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #8365302 -
Attachment is obsolete: true
Attachment #8365302 -
Flags: review?(roc)
Attachment #8367599 -
Flags: review?(roc)
Assignee | ||
Comment 24•11 years ago
|
||
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.
Assignee | ||
Comment 25•11 years ago
|
||
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 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•11 years ago
|
||
(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.
Assignee | ||
Comment 28•11 years ago
|
||
(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•11 years ago
|
||
Alright, if we're ok with a well predicted branch there it makes my life much easier.
Assignee | ||
Comment 31•11 years ago
|
||
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?
Flags: needinfo?(benjamin)
Comment 32•11 years ago
|
||
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.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #8367599 -
Attachment is obsolete: true
Attachment #8367599 -
Flags: review?(roc)
Assignee | ||
Comment 34•11 years ago
|
||
Attachment #8378376 -
Attachment is obsolete: true
Assignee | ||
Comment 35•11 years ago
|
||
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
Attachment #8378390 -
Attachment is obsolete: true
Assignee | ||
Comment 36•11 years ago
|
||
I tried rebasing to see if that fixes the OS X issue...
https://tbpl.mozilla.org/?tree=Try&rev=ef2edeaee12f
Assignee | ||
Comment 37•11 years ago
|
||
What does this look like to you? Is this a real issue? Also, do I need another review?
Flags: needinfo?(bgirard)
Comment 38•11 years ago
|
||
The one above? Looks like a random failure.
We still need review on the widget changes.
Flags: needinfo?(bgirard)
Assignee | ||
Updated•11 years ago
|
Attachment #8379788 -
Flags: review?(roc)
I thought this patch was going to instrument Monitor::Wait, but it doesn't... why not?
Assignee | ||
Comment 40•11 years ago
|
||
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?
Flags: needinfo?(roc)
OK, why not instrument both Monitor::Wait and ReentrantMonitor::Wait?
Flags: needinfo?(roc)
Assignee | ||
Comment 42•11 years ago
|
||
Instrumented Monitor::Wait in addition to ReentrantMonitor:Wait
Attachment #8379788 -
Attachment is obsolete: true
Attachment #8379788 -
Flags: review?(roc)
Attachment #8392280 -
Flags: review?(roc)
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.
Attachment #8392280 -
Flags: review?(roc) → review-
Assignee | ||
Comment 44•11 years ago
|
||
(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
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
Assignee | ||
Comment 46•11 years ago
|
||
(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.
Assignee | ||
Comment 47•11 years ago
|
||
Made the changes specified in the last comment.
https://tbpl.mozilla.org/?tree=Try&rev=adb29cccb8b4
Attachment #8392280 -
Attachment is obsolete: true
Attachment #8393559 -
Flags: review?(roc)
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.
Attachment #8393559 -
Flags: review?(roc)
Attachment #8393559 -
Flags: review?(bgirard)
Attachment #8393559 -
Flags: review+
Comment 49•11 years ago
|
||
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?
Attachment #8393559 -
Flags: review?(bgirard) → review-
Assignee | ||
Comment 50•11 years ago
|
||
(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.
Assignee | ||
Comment 51•11 years ago
|
||
Attachment #8393559 -
Attachment is obsolete: true
Attachment #8396401 -
Flags: review?(bgirard)
Comment 52•11 years ago
|
||
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?
Attachment #8396401 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 53•11 years ago
|
||
Attachment #8396401 -
Attachment is obsolete: true
Attachment #8396501 -
Flags: review?(bgirard)
Assignee | ||
Comment 54•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #8396501 -
Flags: review?(bgirard)
Assignee | ||
Comment 55•11 years ago
|
||
Attachment #8396501 -
Attachment is obsolete: true
Assignee | ||
Comment 56•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8396513 -
Flags: review?(bgirard)
Updated•11 years ago
|
Attachment #8396513 -
Flags: review?(bgirard) → review+
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bgirard)
Comment 57•11 years ago
|
||
Flags: needinfo?(bgirard)
Comment 58•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•