1187062 - We need a stack interface for ubi::Node so we can serialize and deserialize allocation stacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

We want to be able to talk about allocation stacks in our heap graph analyses which means we need to have an interface that can be backed by SavedFrame objects when operating on the live heap and by some deserialized structure when operating on offline heap snapshots.

Comment 1

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

Attachment: Part 0: Make js::Debugger::getObjectAllocationSite return a SavedFrame* rather than a JSObject*

Comment 2

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

Attachment: Part 1: Add JS::ubi::Stack and JS::ubi::StackFrame

Comment 3

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

Attachment: Part 2: Implement concrete JS::ubi::Stack and JS::ubi::StackFrame classes backed by SavedFrame

Comment 4

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

Comment on attachment 8638231 [details] [diff] [review]
Part 2: Implement concrete JS::ubi::Stack and JS::ubi::StackFrame classes backed by SavedFrame

Building on top of these patches, I realize that I need to mess with the interface a little bit more. I was trying so hard to avoid malloc'ing anything, since it shows up a ton on profiles of existing ubi::Node stuff, but I don't think I can really avoid it altogether.

Comment 5

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

Comment on attachment 8638230 [details] [diff] [review]
Part 1: Add JS::ubi::Stack and JS::ubi::StackFrame

See comment 4.

Comment 6

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

Comment on attachment 8638229 [details] [diff] [review]
Part 0: Make js::Debugger::getObjectAllocationSite return a SavedFrame* rather than a JSObject*

I am pretty confident that this can still be reviewed, however :)

Comment 7

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

Attachment: Part 1: Add the JS::ubi::StackFrame interface

Comment 8

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

Attachment: Part 2: Implement a concrete JS::ubi::StackFrame class backed by js::SavedFrame

Comment 9

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

Attachment: Part 3: Add jsapi-tests for JS::ubi::StackFrame

Comment 10

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

You may be wondering why we have two versions of getting the allocation site for a ubi::Node. The reason is that when serializing the heap graph, we only need a stack allocated version because we are going to serialize and forget about it. The census however, needs a heap allocated stack frame because it is making a map of allocation site -> counts. We've seen malloc showing up fairly heavily on census/heap snapshot computations (related to edges/edge ranges which are currently always heap allocated), so I thought it was worth it to have a special case to avoid the heap allocation when possible.

Comment 11

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

Comment on attachment 8646579 [details] [diff] [review]
Part 1: Add the JS::ubi::StackFrame interface

Sorry for the churn! Making one last change to JS::ubi::StackFrame to clear up ownership, lifetimes, and sized-ness!

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8646579 [details] [diff] [review]
Part 1: Add the JS::ubi::StackFrame interface

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

::: js/public/UbiNode.h
@@ +176,5 @@
> +/*** ubi::StackFrame ******************************************************************************/
> +
> +// Depending on the concrete implementation backing an instance of the
> +// JS::ubi::StackFrame interface, it is cheap and easy to return a JSAtom*
> +// (backed by a live SavedFrame) or return const char16_t* (backed by a

"it may be cheaper/easier to return either an X or a Y"? I got a little lost.

@@ +180,5 @@
> +// (backed by a live SavedFrame) or return const char16_t* (backed by a
> +// structure deserialized from an offline heap snapshot) from various
> +// methods. We provide this type alias to handle both cases for the return type
> +// of methods that are simple gets.
> +using AtomOrTwoByteChars = Variant<JSAtom*, const char16_t*>;

I'll have to wait for the concrete subclasses to form an opinion of this Variant craziness, but I trust you have a good reason.

@@ +211,5 @@
> +    // Get this frame's source name. Never null.
> +    virtual AtomOrTwoByteChars source() const = 0;
> +
> +    // Get this frame's function name, if its function is named, or else get the
> +    // its function's inferred display name. Can be null.

"the its"

Maybe "Return this frame's function name if named, otherwise the inferred display name. Can be null."? I don't know. Doesn't matter.

@@ +218,5 @@
> +    // Returns true if this frame's function is system JavaScript running with
> +    // trusted principals, false otherwise.
> +    virtual bool isSystem() const = 0;
> +
> +    // Return true if this frame's funciton is a self-hosted JavaScript builtin,

*function

@@ +259,5 @@
> +    //
> +    // is lossy because we cannot serialize and deserialize the SavedFrame's
> +    // principals in the offline heap snapshot, so JS::ubi::StackFrame
> +    // simplifies the principals check into the boolean isSystem() state. This
> +    // is fine becuase we only expose JS::ubi::Stack to devtools and chrome

*because

@@ +355,5 @@
>      // nullptr is returned.
>      virtual JSCompartment* compartment() const { return nullptr; }
>  
> +    // Return true if the stack where this node's referent was allocated, false
> +    // otherwise.

Uh, whut? I have failed at parsing that English.

I *think* you mean "Return whether this node has a record of the stack from when the referent was allocated." Or something like that. "Return whether this node's referent's allocation stack was captured"?

@@ +359,5 @@
> +    // otherwise.
> +    virtual bool hasAllocationStack() const { return false; }
> +
> +    // Call the given callback functor with this node's referent's allocation
> +    // stack. The allocation stack passed to the callback is a non-owning

Oh, hey, you like the wacky "node's referent's grandmother's dog's reincarnation's..." phrasing too!

@@ +363,5 @@
> +    // stack. The allocation stack passed to the callback is a non-owning
> +    // reference and only has a lifetime of the duration of the callback. This
> +    // must only be called when hasAllocationStack() is true.
> +    virtual void withAllocationStack(StackFrame::Callback& callback) const {
> +        MOZ_CRASH("Concrete classes which have an allocation stack must override all of "

Ok, here's where I go into super-pedant mode. "[which] have an allocation stack" is a restrictive clause, so  s/which/that/. Or do s/which have/with/ instead.

@@ +372,5 @@
> +    // JS::ubi::Stack is valid as long as the UniquePtr exists. A null UniquePtr
> +    // is returned on OOM. This must only be called when hasAllocationStack() is
> +    // true.
> +    virtual UniquePtr<StackFrame> getAllocationStack() const {
> +        MOZ_CRASH("Concrete classes which have an allocation stack must override all of "

You poked the which witch again!

Comment 13

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

Attachment: Part 1: Add the JS::ubi::StackFrame interface

Comment 14

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

Attachment: Part 2: Implement a concrete JS::ubi::StackFrame class backed by js::SavedFrame

Comment 15

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

Attachment: Part 3: Add jsapi-tests for JS::ubi::StackFrame

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8647087 [details] [diff] [review]
Part 1: Add the JS::ubi::StackFrame interface

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

I'm going to look at some of the users before r+'ing this.

::: js/public/UbiNode.h
@@ +200,5 @@
> +    // destructors in subclasses!
> +
> +    // Get a unique identifier for this StackFrame. The identifier is not valid
> +    // across garbage collections.
> +    virtual uintptr_t identifier() const { return reinterpret_cast<uintptr_t>(ptr); }

Is this better than using the address in the calling code? I ask because I was about to suggest that you wrap the return type in a struct or something so that we could tell the hazard analysis that it's an unrooted GCPointer, and thus get free checks that this is never used across a GC. But then I thought that if this were an existing GC pointer type to begin with, you'd already get that, and if you were going through contortions in some caller to avoid using these across a GC then having the type be a GC pointer would implicitly explain why. Normally, an identifier() abstraction would totally make sense to me, but here it seems like it's hiding dangerous detail.

But ptr is a void*, so just returning that wouldn't work. You'd need to switch to Cell* or something. Which, oddly enough, is *not* currently marked as a GCType, which makes no sense whatsoever to me. Um, I'm going to go do a try push *right* *now*...

@@ +316,5 @@
> +
> +    template<typename T>
> +    MOZ_IMPLICIT StackFrame(const JS::Handle<T*>& handle) {
> +        construct(handle.get());
> +    }

But we don't implicitly convert to Value, right? So if they're the same, why are they different? :-)

Perhaps I need to see the uses, but I guess I'm not entirely convinced that this complexity is better than static_casts at the calling sites?

@@ +338,5 @@
> +
> +    // Because StackFrame is just a vtable pointer and an instance pointer, we
> +    // can memcpy everything around instead of making concrete classes define
> +    // virtual constructors. See the comment above Node's copy constructor for
> +    // more details; that comment applies here as well.

mempcying vtables. evil. just pure evil.

@@ +348,5 @@
> +        memcpy(storage.u.mBytes, rhs.storage.u.mBytes, sizeof(storage.u));
> +        return *this;
> +    }
> +
> +    bool operator==(const StackFrame& rhs) const { return base()->ptr == rhs.base()->ptr; }

I hope StackFrame, at least, is a Cell subclass?

@@ +415,5 @@
> +  public:
> +    static void construct(void* storage, void*) { new (storage) ConcreteStackFrame(nullptr); }
> +
> +    uintptr_t identifier() const { return 0; }
> +    void trace(JSTracer* trc) override { }

Should this be 'final' instead of 'override'? I'm not up to speed on these new-fangled virtual variants.

Comment 17

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8647089 [details] [diff] [review]
Part 3: Add jsapi-tests for JS::ubi::StackFrame

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

::: js/src/jsapi-tests/testUbiNode.cpp
@@ +6,5 @@
>  
>  #include "jsapi-tests/tests.h"
>  
> +#include "builtin/TestingFunctions.h"
> +#include "vm/SavedFrame.h"

Hm, I thought this chunk of includes went with the js/ includes. Does this pass make check-style? (Run from within objdir or something)

@@ +112,5 @@
> +checkString(const char* expected, F fillBufferFunction, G stringGetterFunction)
> +{
> +    auto expectedLength = strlen(expected);
> +    char16_t buf[1024];
> +    if (fillBufferFunction(mozilla::RangedPtr<char16_t>(buf, 1024), 1024) != expectedLength ||

Couldn't this be

   if (fillBufferFunction(mozilla::RangedPtr<char16_t>(buf), sizeof(buf)) != expectedLength ||

? (Letting RangedPtr figure out the size, and using sizeof for fillBufferFunction.)

@@ +161,5 @@
> +                              return ubiFrame.source(ptr, length);
> +                          },
> +                          [&] {
> +                              return ubiFrame.source();
> +                          }));

Whoa, doing the lambda lambada!

Comment 18

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8647087 [details] [diff] [review]
Part 1: Add the JS::ubi::StackFrame interface

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

::: js/public/UbiNode.h
@@ +200,5 @@
> +    // destructors in subclasses!
> +
> +    // Get a unique identifier for this StackFrame. The identifier is not valid
> +    // across garbage collections.
> +    virtual uintptr_t identifier() const { return reinterpret_cast<uintptr_t>(ptr); }

As discussed on IRC, I don't think Cell is the right answer. It'll have to be specific annotations, and we can leave it for a followup.

@@ +316,5 @@
> +
> +    template<typename T>
> +    MOZ_IMPLICIT StackFrame(const JS::Handle<T*>& handle) {
> +        construct(handle.get());
> +    }

As per IRC, we'll leave these out for now and use a static_cast at the one site needed. For now, at least; we can add them back if needed.

@@ +348,5 @@
> +        memcpy(storage.u.mBytes, rhs.storage.u.mBytes, sizeof(storage.u));
> +        return *this;
> +    }
> +
> +    bool operator==(const StackFrame& rhs) const { return base()->ptr == rhs.base()->ptr; }

Uh, why was I asking if StackFrame is a Cell subclass? It's in this patch.

And no, it's not a Cell subclass. But it's a Traceable subclass. I ought to make Traceable into a GCPointer for the analysis. Though I'm not sure that'll work right now.

Comment 19

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

Attachment: Part 1: Add the JS::ubi::StackFrame interface

Comment 20

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

Attachment: Part 2: Implement a concrete JS::ubi::StackFrame class backed by js::SavedFrame

Comment 21

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

Attachment: Part 3: Add jsapi-tests for JS::ubi::StackFrame

Comment 22

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

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

Comment 23

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

Attachment: Part 1: Add the JS::ubi::StackFrame interface

Comment 24

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

Fixed the errors complained about by static analysis. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb9b4f51b30c

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5014bce76b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae29da8d7f80
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ffe3eacef1
https://hg.mozilla.org/integration/mozilla-inbound/rev/638c65c33cfa

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5014bce76b3
https://hg.mozilla.org/mozilla-central/rev/ae29da8d7f80
https://hg.mozilla.org/mozilla-central/rev/40ffe3eacef1
https://hg.mozilla.org/mozilla-central/rev/638c65c33cfa