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

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Assignee: :fitzgen

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e0509b916e

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Jim Blandy :jimb]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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+.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2bbd01b0035

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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"!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that this patch depends on the patch in bug 961323, which is also checkin-needed.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d639c8e4b889

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d639c8e4b889