Collect interleaved stacks in the Background Hang Reporter

RESOLVED FIXED in Firefox 57

Status

()

Core
XPCOM
RESOLVED FIXED
3 months ago
3 days ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

(Depends on: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

In bug 1357829 I changed the background hang reporter to use the profiler's stackwalking code to collect native hang stacks.

It would be even nicer if we could re-use the profiler's stack interleaving logic to generate fully interleaved stacks from background hang reports. This bug tracks doing that in a privacy preserving manner.
Depends on: 1378055
I've started working on this.
Assignee: michael → n.nethercote
Depends on: 1379565
I'm making progress on blockers for this, untangling some messes.
See Also: → bug 1380081
Depends on: 1379933
Created attachment 8885659 [details] [diff] [review]
Introduce ProfilerStackCollector

mystor, please take a look at ProfilerStackCollector and see if it fits your
needs.

There are two things I haven't addressed yet.

- Privacy concerns about the dynamic strings. Is the dynamic string always
  privacy-sensitive? If so, that would make things easy, because
  BHRStackCollector could just always ignore the aStr argument.

- I'm not sure what additional processing of JITReturnAddr frames is required.
Attachment #8885659 - Flags: feedback?(michael)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Created attachment 8885659 [details] [diff] [review]
> Introduce ProfilerStackCollector

It's possible this patch should go in a bug report that blocks this one, given that this patch introduces machinery to support the collection of interleaved stacks, but doesn't actually collect them.
> It's possible this patch should go in a bug report that blocks this one,
> given that this patch introduces machinery to support the collection of
> interleaved stacks, but doesn't actually collect them.

I filed bug 1380286.
Attachment #8885659 - Attachment is obsolete: true
Attachment #8885659 - Flags: feedback?(michael)
Depends on: 1380286
Assignee: n.nethercote → michael
Depends on: 1380081
Depends on: 1386751
Created attachment 8895459 [details] [diff] [review]
Part 1: Modify HangStack to allow it to also contain native frame entries

This one is a bit large, as changing one piece had large impacts on the rest of BHR. 

If it's too much for one patch I can try to break it up into smaller patches.

MozReview-Commit-ID: AfrtWfLUi2l
Attachment #8895459 - Flags: review?(nfroyd)
Created attachment 8895460 [details] [diff] [review]
Part 2: Add a method for reading module information for native stack frames

MozReview-Commit-ID: 7gzFhygwXx4
Attachment #8895460 - Flags: review?(nfroyd)
Created attachment 8895461 [details] [diff] [review]
Part 3: Expose computed modules through .modules getter

MozReview-Commit-ID: F6aDnlwr9jt
Attachment #8895461 - Flags: review?(nfroyd)
Blocks: 1388818
Comment on attachment 8895461 [details] [diff] [review]
Part 3: Expose computed modules through .modules getter

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

::: toolkit/components/backgroundhangmonitor/HangDetails.cpp
@@ +145,4 @@
>  nsHangDetails::GetModules(JSContext* aCx, JS::MutableHandleValue aVal)
>  {
>    size_t length = 0;
>    JS::RootedObject retObj(aCx, JS_NewArrayObject(aCx, length));

Now that we're putting information in here, maybe we should do instead:

  auto& modules = mDetails.mStack.GetModules();
  size_t length = modules.Length();
  ...JS_NewArrayObject called with the correct length...

  for (size_t i = 0; i < length; ++i) {
    ...
  }
Attachment #8895461 - Flags: review?(nfroyd) → review+
Comment on attachment 8895460 [details] [diff] [review]
Part 2: Add a method for reading module information for native stack frames

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

I think this is all reasonable?

::: toolkit/components/backgroundhangmonitor/HangStack.cpp
@@ +92,5 @@
> +
> +} // anonymous namespace
> +
> +void
> +HangStack::ReadModuleInformation()

Documentation says mModules should be empty when this function is called, maybe MOZ_ASSERT this?

@@ +109,5 @@
> +  rawModules.SortByAddress();
> +
> +  size_t frameIdx = 0;
> +  for (size_t i = 0; i < rawModules.GetSize(); ++i) {
> +    const SharedLibrary& info= rawModules.GetEntry(i);

Nit: spaces around `=`.

@@ +127,5 @@
> +        // ModOffset one. mModules.Length() will be the index of the module when
> +        // we append it below, and we set moduleReferenced to true to ensure
> +        // that we do.
> +        moduleReferenced = true;
> +        uint32_t module = mModules.Length();

Maybe these computations should go after checking the offset for validity, so we don't wind up declaring the module used and then not inserting it into the list because the offset was > UINT32_MAX?  (A bit of a weird situation, to be sure, but worth handling, no?)

@@ +129,5 @@
> +        // that we do.
> +        moduleReferenced = true;
> +        uint32_t module = mModules.Length();
> +        size_t offset = frame->AsPC() - moduleStart;
> +        if (NS_WARN_IF(offset > UINT32_MAX)) {

Should probably explicitly compute offset as a uint64_t so you don't get warnings here for the tautological comparison on 32-bit platforms?
Attachment #8895460 - Flags: review?(nfroyd) → review+
Comment on attachment 8895459 [details] [diff] [review]
Part 1: Modify HangStack to allow it to also contain native frame entries

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

I think I would prefer a smaller set of patches if possible.

::: toolkit/components/backgroundhangmonitor/HangStack.h
@@ +88,5 @@
> +      MOZ_ASSERT(mKind == Kind::STRING);
> +      return mString;
> +    }
> +
> +    const char* const& AsString() const {

Your dedication to const-correctness is to be applauded.
Attachment #8895459 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #10)
> Documentation says mModules should be empty when this function is called,
> maybe MOZ_ASSERT this?

Fair.

I don't MOZ_ASSERT much in BHR because we don't run BHR when DEBUG is set (bug 979069), so it isn't _exactly_ tested. Perhaps I'll MOZ_RELEASE_ASSERT it.
MOZ_RELEASE_ASSERT has a bunch of potential for badness, though it would definitely catch the error.  Maybe just force-clearing the array instead?
(In reply to Nathan Froyd [:froydnj] from comment #13)
> MOZ_RELEASE_ASSERT has a bunch of potential for badness, though it would
> definitely catch the error.  Maybe just force-clearing the array instead?

Sure, I can do that.
Created attachment 8896000 [details] [diff] [review]
Part 1A: Allow HangStack to contain raw PCs and Module offsets

The HangStack previously consisted of an array of const char* pointers into its
backing string buffer, which represented pseudostack entries. With interleaved
stacks, it is now possible for the stack to contain raw unresolved program
counters (Kind::PC), and module/offset pairs (Kind::MODOFFSET). To do this, we
use a discriminated union, and make the backing array use the discriminated
union instead of const char*s.

The code cannot use mozilla::Variant<const char*, uintptr_t, Module>
unfortuantely, as we cannot use the implementation of ParamTraits for Variant in
HangStack's ParamTraits implementation.

When deserializing a HangStack over IPC, we need to read the string frame
entries into the backing string buffer, and generate const char* entries for
each of the strings which we read in over IPC. The default implementation of
ParamTraits wouldn't give us access to the enclusing HangStack object while
deserializing each individual entry, so we couldn't use it. In fact, Entries
don't have ParamTraits implemented for them at all, and can only be sent over
IPC as part of a HangStack due to this dependency.

MozReview-Commit-ID: KrLn9e9OHAR
Attachment #8896000 - Flags: review?(nfroyd)
Attachment #8895459 - Attachment is obsolete: true
Created attachment 8896001 [details] [diff] [review]
Part 1B: Remove nsIHangDetails.pseudoStack, replace ProcessedStack w/ new HangStack type

Previously there were two stack objects on each HangDetails object: mStack and
mPseudoStack. mStack was a Telemetry::ProcessedStack, while mPseudoStack was a
HangStack. After the changes in part 1A, HangStack can now contain all of the
information of both the old HangStack and ProcessedStack, so the mPseudoStack
field is renamed to mStack, and the old mStack field is removed.

This patch also implements the new GetStack getter, which generates the JS data
format for the new HangStack type.

MozReview-Commit-ID: C27ooyw70oD
Attachment #8896001 - Flags: review?(nfroyd)
Created attachment 8896002 [details] [diff] [review]
Part 1C: Collect interleaved stacks w/ ProfilerStackCollector API in ThreadStackHelper

This new API was added by njn in bug 1380286, and provides both pseudostack and
native stack entries to the consumer of the API.

This patch changes ThreadStackHelper to use this new API instead of the previous
one, and use it to collect the frames directly into HangStack objects.

MozReview-Commit-ID: JihfcBL5Cdl
Attachment #8896002 - Flags: review?(nfroyd)
Attachment #8896000 - Flags: review?(nfroyd) → review+
Attachment #8896001 - Flags: review?(nfroyd) → review+
Comment on attachment 8896002 [details] [diff] [review]
Part 1C: Collect interleaved stacks w/ ProfilerStackCollector API in ThreadStackHelper

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

Thanks for splitting these patches up, things made a lot more sense this way.
Attachment #8896002 - Flags: review?(nfroyd) → review+

Comment 19

3 days ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5f22010d45
Part 1: Add interleaved stack functionality to HangStack, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6959119152
Part 2: Add a method for reading module information for native stack frames, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f65afbc48a
Part 3: Expose computed modules through .modules getter, r=froydnj

Comment 20

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a5f22010d45
https://hg.mozilla.org/mozilla-central/rev/5a6959119152
https://hg.mozilla.org/mozilla-central/rev/70f65afbc48a
Status: NEW → RESOLVED
Last Resolved: 3 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

3 days ago
Depends on: 1391126
You need to log in before you can comment on or make changes to this bug.