899782 - Add PseudoStack entries back into Windows current-thread backtraces

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 3

[Benoit Girard (:BenWa)]

(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

[Benoit Girard (:BenWa)]

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.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(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

[(no longer active)]

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

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Try build in comment 8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/51170e07d7e9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51170e07d7e9