Expose a method to JS for find the shortest retaining paths of some nodes in a heap snapshot

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee: :fitzgen
Created attachment 8718398 [details] [diff] [review]
Expose a method to JS for find the shortest retaining paths of some nodes in a heap snapshot

This commit adds the `computeShortestPaths` method to the `HeapSnapshot` webidl
interface. It implements this new method on the
`mozilla::devtools::HeapSnapshot` class.

r=bz for the webidl bits
r=jimb for the devtools bits
Attachment #8718398 - Flags: review?(jimb)
Attachment #8718398 - Flags: review?(bzbarsky)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e0509b916e
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Blocks: 1149385
Depends on: 961323
Blocks: 961331
Comment on attachment 8718398 [details] [diff] [review]
Expose a method to JS for find the shortest retaining paths of some nodes in a heap snapshot

The IDL parts look fine.

The implementation has at least one problem, which is the use of JS_AtomizeAndPinUCString.  That call will _permanently_ pin the memory of that string in that runtime.  So if we dynamically generate edge names this seems like a bad idea.  This is why we generally try to avoid having people write JSAPI in Gecko...

In general, I think that whole "build the result" section could be replaced with this bit of IDL:

  dictionary HeapSnapshotShortestPathEntry {
    required NodeId predecessor;
    required DOMString? edge;
  };

and then this C++ (doing infallible appends to the arrays; if you want them fallible it's not that hard to adjust accordingly by padding fallible() to AppendElement() and null-checking its return value):

  nsTArray<nsTArray<HeapSnapshotShortestPathEntry>> paths;
  bool ok = shortestPaths.forEachPath(range.front(), [&](JS::ubi::Path& path) {
    nsTArray<HeapSnapshotShortestPathEntry>& pathValues =
      *paths.AppendElement();
    for (JS::ubi::BackEdge* edge : path) {
      HeapSnapshotShortestPathEntry& pathPart = *pathValues.AppendElement();
      pathPart.mPredecessor = edge->predecessor().identifier();
      if (edge->name()) {
        pathPart.mEdge = edge->name().get();
      } else {
        SetDOMStringToNull(pathPart.mEdge);
      }
    }

    return true;
  });

  // deal with !ok here

  JS::RootedValue pathsVal(cx);
  if (!ToJSValue(cx, paths, &pathsVal)) {
    rv.NoteJSContextException();
    return;
  }

In general, rv.NoteJSContextException() is what you want to use for the cases when JSAPI that returns false on fail returned false, since presumably it set up an exception already, right?

Anyway, r=me with at least JS_AtomizeAndPinUCString replaced by something that won't permanently leak the memory...
Attachment #8718398 - Flags: review?(bzbarsky) → review+

Comment 4

3 years ago
Comment on attachment 8718398 [details] [diff] [review]
Expose a method to JS for find the shortest retaining paths of some nodes in a heap snapshot

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

Clearing review, since I'd like to see the code after bz's suggestion is implemented.

::: devtools/shared/heapsnapshot/HeapSnapshot.cpp
@@ +636,5 @@
> +    auto ccrt = CycleCollectedJSRuntime::Get();
> +    MOZ_ASSERT(ccrt);
> +    auto rt = ccrt->Runtime();
> +    MOZ_ASSERT(rt);
> +    JS::AutoCheckCannotGC nogc(rt);

This is all entirely meaningless since we're operating on a snapshot, right? We take the value out of the scope of `nogc` anyway. I wonder if it wouldn't be better for `ShortestPaths::Create` to only require the `AutoCheckCannotGC` if any of the start and targets are "live" nodes.
Attachment #8718398 - Flags: review?(jimb)
Depends on: 1247743
Created attachment 8718558 [details] [diff] [review]
Expose a method to JS for find the shortest retaining paths of some nodes in a heap snapshot

Removed the last iteration's pinning behavior via exposing atomizing without
pinning over JSAPI in bug 1247743. The atomizing is valuable because many, many
of the edge names will be duplicates.

As discussed on IRC, using the dictionary approach becomes a little bit messier
with atomizing: we would need to replace the DOMString member with an any
member, and rooting becomes a little convoluted. Because of that, I've decided
to keep the code that is already written for generating the resulting Map
object.

Carrying over bz's r+.
Attachment #8718558 - Flags: review?(jimb)
Attachment #8718558 - Flags: review+
> This is why we generally try to avoid having people write JSAPI in Gecko...

To be clear, that's because getting jsapi usage right is so insanely hard.  This is a problem with JSAPI, not with "people"!

Updated

3 years ago
Attachment #8718558 - Flags: review?(jimb) → review+
Note that this patch depends on the patch in bug 961323, which is also checkin-needed.
Keywords: checkin-needed
Attachment #8718398 - Attachment is obsolete: true
Blocks: 1248085

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d639c8e4b889
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.