1358619 - Only pause the target thread once when collecting pseudo and native stacks for BHR

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1346415 BHR now collects native stacks at the same time as pseudostacks when on nightly. In this situation, however, the target thread is paused twice, first for the pseudostack collection, and second for the native stack. 

We should only pause the thread once for collecting both stacks.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Fetch the stack and native stack within the same pause of the target thread

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8860519 [details] [diff] [review]
Fetch the stack and native stack within the same pause of the target thread

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

Sorry for the delay in this review.  We can talk about some of the decisions below before you land, but I don't think I need to see the revised patch, so r=me with changes.

::: xpcom/threads/BackgroundHangMonitor.cpp
@@ +355,5 @@
> +          if (currentThread->mStats.mNativeStackCnt >= kMaximumNativeHangStacks) {
> +            currentThread->mNativeHangStack.clear();
> +          } else {
> +            currentThread->mStats.mNativeStackCnt += 1;
> +          }

Can we rewrite this to be more verbose, but to do just the work required?  e.g.

if (currentThread->mStats.mNativeStackCnt >= kMaximumNativeHangStacks) {
  currentThread->mStackHelper.GetPsuedoStack(...);
} else {
  currentThread->mStackHelper.GetPseudoAndNativeStack(...);
  currentThread->mStats.mNativeStackCnt += 1;
}

::: xpcom/threads/ThreadStackHelper.cpp
@@ +161,5 @@
>  };
>  } // namespace
>  
>  void
>  ThreadStackHelper::GetStack(Stack& aStack)

Perhaps this should now be GetPseudoStack for similarity with GetPseudoAndNativeStack?

::: xpcom/threads/ThreadStackHelper.h
@@ +127,4 @@
>  
>  private:
> +  // NOTE: Both aStack and aNativeStack may be nullptr. If they are, they will
> +  // not be filled in.

This sentence reads peculiarly: is each of them permitted to be nullptr separately, or do they have to both be nullptr simultaneously?  How about:

// Fill in the passed aStack and aNativeStack datastructures with backtraces.
// If only aStack needs to be collected, nullptr may be passed for
// aNativeStack, and vice versa.

@@ +127,5 @@
>  
>  private:
> +  // NOTE: Both aStack and aNativeStack may be nullptr. If they are, they will
> +  // not be filled in.
> +  void GetStackInternal(Stack* aStack, NativeStack* aNativeStack);

WDYT about calling this "GetStacksInternal"?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Fetch the stack and native stack within the same pause of the target thread

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77db7b6e3dee
Fetch the stack and native stack within the same pause of the target thread, r=froydnj

Comment 5

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/77db7b6e3dee