Closed
Bug 899782
Opened 12 years ago
Closed 11 years ago
Add PseudoStack entries back into Windows current-thread backtraces
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
Attachments
(1 file, 1 obsolete file)
2.35 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
The implementation of TableTicker::doNativeBacktrace() currently uses a workaround when a thread obtains its own backtrace on Windows. This is to avoid the multithreading shenanigans that need to take place to obtain a CONTEXT using NS_StackWalk() in this scenario. Unfortunately this also means that we don't have stack pointers to use for merging in the PseudoStack.
Fix this up so that we can include PseudoStack data profiler_get_backtrace() results.
Assignee | ||
Comment 1•11 years ago
|
||
I've run a bunch of tests using both the existing NS_StackWalk and an NS_StackWalk that avoids the multithreading.
For a call stack depth of 30, the mean single-threaded perf improvement is 7.7%. In terms of time, we're talking 17ns. There is also higher variability in the test implementation, particularly when multiple threads are walking their own stacks. This is probably due to the fact that a Windows message queue is FIFO whereas a CRITICAL_SECTION is unfair.
Furthermore, the fact that we require mutual exclusion around StackWalk64 makes it a candidate for deadlock.
Given these numbers and the other challenges involved with using StackWalk64 in the profiler, I think we're okay to continue to use it as-is.
Assignee | ||
Comment 2•11 years ago
|
||
Based on my remarks in comment 1, I think we're safe to use the NS_StackWalk implementation as-is. This patch removes the alternate code path.
Attachment #812821 -
Flags: review?(bgirard)
Comment 3•11 years ago
|
||
(In reply to Aaron Klotz [:aklotz] from comment #1)
> Furthermore, the fact that we require mutual exclusion around StackWalk64
> makes it a candidate for deadlock.
>
This is concerning. If the main thread is doing a sync unwind and we try to sample will we deadlock firefox?
Comment 4•11 years ago
|
||
Comment on attachment 812821 [details] [diff] [review]
Remove special code path for sync stacks on Windows
Review of attachment 812821 [details] [diff] [review]:
-----------------------------------------------------------------
I'm ok with this conditional that we don't introduce a possible deadlock.
Attachment #812821 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #3)
> (In reply to Aaron Klotz [:aklotz] from comment #1)
> > Furthermore, the fact that we require mutual exclusion around StackWalk64
> > makes it a candidate for deadlock.
> >
>
> This is concerning. If the main thread is doing a sync unwind and we try to
> sample will we deadlock firefox?
With the current NS_StackWalk there is no concern because all requests are handled by a thread that is spun up specifically to do the stack walking, and that thread is not registered with the profiler.
The deadlock issue would only happen if I had gone ahead with making sync unwind work without this additional thread, which I don't think is worth doing.
Comment 6•11 years ago
|
||
(In reply to comment #5)
> (In reply to Benoit Girard (:BenWa) from comment #3)
> > (In reply to Aaron Klotz [:aklotz] from comment #1)
> > > Furthermore, the fact that we require mutual exclusion around StackWalk64
> > > makes it a candidate for deadlock.
> > >
> >
> > This is concerning. If the main thread is doing a sync unwind and we try to
> > sample will we deadlock firefox?
> With the current NS_StackWalk there is no concern because all requests are
> handled by a thread that is spun up specifically to do the stack walking, and
> that thread is not registered with the profiler.
No, things work exactly the other way around. We only use the additional thread code path for printing assertion stacks and whatnot.
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> (In reply to comment #5)
> > (In reply to Benoit Girard (:BenWa) from comment #3)
> > > (In reply to Aaron Klotz [:aklotz] from comment #1)
> > > > Furthermore, the fact that we require mutual exclusion around StackWalk64
> > > > makes it a candidate for deadlock.
> > > >
> > >
> > > This is concerning. If the main thread is doing a sync unwind and we try to
> > > sample will we deadlock firefox?
> > With the current NS_StackWalk there is no concern because all requests are
> > handled by a thread that is spun up specifically to do the stack walking, and
> > that thread is not registered with the profiler.
>
> No, things work exactly the other way around. We only use the additional
> thread code path for printing assertion stacks and whatnot.
We're talking about the case where a thread is calling NS_StackWalk on itself for sync unwinding. The additional code path applies to this case.
Assignee | ||
Comment 8•11 years ago
|
||
I had to revise this patch because I realized that we also had to provide a 0 thread ID to NS_StackWalk in the sync stack case. This indicates to NS_StackWalk that it will be walking the current thread's stack.
https://tbpl.mozilla.org/?tree=Try&rev=2866d6df1537
Attachment #812821 -
Attachment is obsolete: true
Attachment #817274 -
Flags: review?(bgirard)
Updated•11 years ago
|
Attachment #817274 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•