Add PseudoStack entries back into Windows current-thread backtraces

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla27
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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

5 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

5 years ago
Created attachment 812821 [details] [diff] [review]
Remove special code path for sync stacks on Windows

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

5 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

5 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

5 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

5 years ago
Created attachment 817274 [details] [diff] [review]
win32fix2

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

5 years ago
Attachment #817274 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/51170e07d7e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.