1050445 - Add support for tracking callstacks to BlockingResourceBase

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

This is a followup to bug 1049068 where we removed callstack handling from the DeadlockDetector.

Previously callstack tracking required a tracemalloc build. In this incarnation I'd like to track the callstacks in BlockingResourceBase itself, this will allow us to avoid grabbing the DeadlockDetector master lock for cases where this is the first lock a thread is acquiring.

The implementation would probably just use NS_StackWalk directly and store an array of addresses. When a deadlock is detected we can then use the AddressCache (or whatever it's called now) to convert the addresses to proper names, rather than performing the lookup at the time of locking like the previous implementation did.

Vaguely here's what I'm thinking:

BlockingResourceBase:
  #ifdef MOZ_TRACK_CALLSTACKS
    void* mCurrentStack[];
    void* mFirstStack[];
  #endif

BlockingResourceBase::Acquire:
    GrabLock();
  #ifdef MOZ_TRACK_CALLSTACKS
    mCurrentStack = GetCallstack();
    if (!mFirstStack)
        mFirstStack = mCurrentStack;
  #endif

BlockingResourceBase::Release:
  #ifdef MOZ_TRACK_CALLSTACKS
    Clear(mCurrentStack);
  #endif
  LetGoOfTheLock();

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Add AcquisitionState typedef

This prepares for having an alternate acquisition state definition.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Add ClearAcquisitionState

This adds a ClearAcquisitionState function that is used instead of setting the acquisition state to false directly.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 3: Add stack trace holders

This patch introduces the option to capture callstacks and plugs in the alternate AcquisitionState definition as a nsAutoTArray<void*, 24>.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 4: Take stack snapshots

Add stack snapshots for each acquisition context by using NS_StackWalk.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 5: Print stacks

This adds stack printing in deadlock scenarious using CodeAddressService. The implementation is essentially lifted from nsTraceRefcnt.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8471725 [details] [diff] [review]
Part 3: Add stack trace holders

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

::: xpcom/glue/BlockingResourceBase.cpp
@@ +43,5 @@
> +#ifdef MOZ_CALLSTACK_DISABLED
> +  #define IS_ACQUIRED(callstack) (callstack)
> +#else
> +  #define IS_ACQUIRED(callstack) (!callstack.IsEmpty())
> +#endif

I feel like this would be better as a private member function.

::: xpcom/glue/BlockingResourceBase.h
@@ +101,3 @@
>    typedef bool AcquisitionState;
> +#else
> +  typedef nsAutoTArray<void*, 24> AcquisitionState;

This is going to be quite some overhead for each and every BlockingResourceBase.  I think I'd just make this nsTArray<void*> unless we find that the extra space really helps.  (The previous version had heap-allocated stacks IIRC, so I don't think we need to change that here.)

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8471725 [details] [diff] [review]
Part 3: Add stack trace holders

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

::: xpcom/glue/BlockingResourceBase.cpp
@@ +43,5 @@
> +#ifdef MOZ_CALLSTACK_DISABLED
> +  #define IS_ACQUIRED(callstack) (callstack)
> +#else
> +  #define IS_ACQUIRED(callstack) (!callstack.IsEmpty())
> +#endif

Good point, the fewer macros the better.

::: xpcom/glue/BlockingResourceBase.h
@@ +101,3 @@
>    typedef bool AcquisitionState;
> +#else
> +  typedef nsAutoTArray<void*, 24> AcquisitionState;

I'm somewhat following the DMD implementation which uses a fixed sized array and limits the stacks to 24 calls.

My thinking here is that this will only be enabled when someone hits a deadlock, at that point we don't really care about memory but we probably care about performance.

Performance implications:
nsAutoTArray - fixed size, no allocations, no deallocations as long as we stay within the size

nsTArray - allocates on first append, will double storage thereafter (so at 2, 4, 8, 16) involving allocation and copying of elements. Will deallocate storage when cleared, so each time we lock and unlock we'll go through the whole alloc/dealloc cycle again.

Memory implications:
From previous profiling I think I saw the number of live mutexes peak around 1000.

nsAutoTArray - this adds ~384 bytes per mutex (on 64-bit), so we're looking at a grand total of ~384K overhead

nsTArray - in best case scenario we say every lock is unlocked, so we're only keeping track of firstSeen. We'll also assume a 24 stack limit. That means ~1/2 of the nsAutoTArray overhead, or ~192K

Comment 8

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #7)
> nsTArray - in best case scenario we say every lock is unlocked, so we're
> only keeping track of firstSeen. We'll also assume a 24 stack limit. That
> means ~1/2 of the nsAutoTArray overhead, or ~192K

And since nsTArray will actually allocate 32 entries instead of 24 the overhead is more like ~256K.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #7)
> Comment on attachment 8471725 [details] [diff] [review]
> Part 3: Add stack trace holders
> 
> Review of attachment 8471725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/BlockingResourceBase.cpp
> @@ +43,5 @@
> > +#ifdef MOZ_CALLSTACK_DISABLED
> > +  #define IS_ACQUIRED(callstack) (callstack)
> > +#else
> > +  #define IS_ACQUIRED(callstack) (!callstack.IsEmpty())
> > +#endif
> 
> Good point, the fewer macros the better.
> 
> ::: xpcom/glue/BlockingResourceBase.h
> @@ +101,3 @@
> >    typedef bool AcquisitionState;
> > +#else
> > +  typedef nsAutoTArray<void*, 24> AcquisitionState;
> 
> I'm somewhat following the DMD implementation which uses a fixed sized array
> and limits the stacks to 24 calls.
> 
> My thinking here is that this will only be enabled when someone hits a
> deadlock, at that point we don't really care about memory but we probably
> care about performance.

I guess if we have the stack tracing turned on, we're eventually going to allocate space for the arrays anyway at some point.  Might as well allocate them upfront, then.  nsAutoTArray wfm, then.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8471732 [details] [diff] [review]
Part 5: Print stacks

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

::: xpcom/glue/BlockingResourceBase.cpp
@@ +122,5 @@
>    return maybeImminent;
>  }
>  
> +#ifndef MOZ_CALLSTACK_DISABLED
> +class CodeAddressServiceWriter MOZ_FINAL

It seems like you should factor all of this out of nsTraceRefCnt into a new header if it is really just the same code as exists already.

I assume this ends up producing a ton of stack traces?  Also, I think Kyle told me the code address service is broken on B2G or something, so watch out for that...

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8471729 [details] [diff] [review]
Part 4: Take stack snapshots

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

Seems ok, but I don't feel like I have much expertise for this patch.

Comment 12

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Comment on attachment 8471732 [details] [diff] [review]
> Part 5: Print stacks
> 
> Review of attachment 8471732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/BlockingResourceBase.cpp
> @@ +122,5 @@
> >    return maybeImminent;
> >  }
> >  
> > +#ifndef MOZ_CALLSTACK_DISABLED
> > +class CodeAddressServiceWriter MOZ_FINAL
> 
> It seems like you should factor all of this out of nsTraceRefCnt into a new
> header if it is really just the same code as exists already.

I'll file a followup for that, it seems like there could be further refactoring of CodeAddressService to make it easier to use.

Comment 13

[Eric Rahm [:erahm]]

tbpl is somewhat busted, but the try looks ok: https://tbpl.mozilla.org/?tree=Try&rev=b954908415ae

Comment 14

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a6466f92f173
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d41b55d558
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fddd58eca98b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8940df19f856
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1d26661162

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6466f92f173
https://hg.mozilla.org/mozilla-central/rev/a4d41b55d558
https://hg.mozilla.org/mozilla-central/rev/fddd58eca98b
https://hg.mozilla.org/mozilla-central/rev/8940df19f856
https://hg.mozilla.org/mozilla-central/rev/0a1d26661162