Closed Bug 1358619 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(1 file, 1 obsolete file)

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 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"?
Attachment #8860519 - Flags: review?(nfroyd) → review+
Attachment #8860519 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
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
https://hg.mozilla.org/mozilla-central/rev/77db7b6e3dee
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: