1443592 - Consolidate hasCachedSavedFrame flag handling for SpiderMonkey stack frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jim Blandy :jimb]

(This is a sub-bug for some cleanup work needed for 1438121, fixing async stack capture.)

The way we check and set the 'hasCachedSavedFrame' flag on stack frames is too complicated, and leads to bugs and confusion.

- On-stack replacement changes Baseline frames to Ion frames, changing the frame address stored in the LiveSavedFramesCache without clearing the flag, making it impossible to tighten assertions on the cache's activities - say, that a frame with its flag set does indeed appear in the cache.

- Although wasm::DebugFrames do have a hasCachedSavedFrame flag, it is not used because FrameIter::hasCachedSavedFrame screens out wasm frames entirely.

- AbstractFramePtr has methods to get and set the flag, but they are not appropriate for every type of frame that AbstractFramePtr can represent.

Instead, LiveSavedFrameCache::FramePtr should represent a pointer to any frame that can carry a flag, and no other; and the flag access methods should be moved to that type.

Comment 1

[Jim Blandy :jimb]

Attachment: Part 1: Introduce opaque LiveSavedFrameCache::Key type for cache keys.

Comment 2

[Jim Blandy :jimb]

Attachment: Part 2: Move hasCachedSavedFrame flag access from FrameIter to LiveSavedFrameCache::FramePtr.

Comment 3

[Jim Blandy :jimb]

Attachment: Part 3: Change LiveSavedFrameCache::find to take a FramePtr, not a FrameIter.

Comment 4

[Jim Blandy :jimb]

Attachment: Part 4: Enable saved frame caching for wasm debug frames.

Comment 5

[Jim Blandy :jimb]

Attachment: Part 5: Move LiveSavedFrameCache::getFramePtr to FramePtr::make.

Comment 6

[Jim Blandy :jimb]

Attachment: Part 6: Remove hasCachedSavedFrame accessors from AbstractFramePtr.

Comment 7

[Jim Blandy :jimb]

Attachment: Part 7: Use jit::CommonFrameLayout to store the hasCachedSavedFrame flag for Baseline frames.

Comment 8

[Jim Blandy :jimb]

Jan, this is essentially the same patch I asked you to review earlier over in bug 1438121. But it turns out that patch was wrong: there was yet another place that knew about which kind of pointer should be used for which kind of frame, which I had neglected to fix. The prior patches in this bug clean all that up, and make LiveSavedFrameCache::FramePtr the sole authority on such things, so the change is more obvious.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8956717 [details] [diff] [review]
Part 4: Enable saved frame caching for wasm debug frames.

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

I'm not familiar with the wasm debugging code, but if this works fine it seems reasonable.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8956720 [details] [diff] [review]
Part 7: Use jit::CommonFrameLayout to store the hasCachedSavedFrame flag for Baseline frames.

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

Still beautiful. r=me with nits below addressed.

::: js/src/vm/Stack-inl.h
@@ +996,2 @@
>  
>      return mozilla::Nothing();

I think this code is unreachable because we handled all cases. There's AbstractFramePtr::isScriptFrameIterData, but bug 1443583 is removing that.

MOZ_CRASH(...) here would be nice to document this and then we know to update this code when we add a new AbstractFramePtr variant.

::: js/src/vm/Stack.h
@@ +2316,5 @@
> +        return true;
> +
> +    if (jitFrame.isIonScripted())
> +        // Only the bottom of a group of inlined Ion frames is a physical frame.
> +        return ionInlineFrames_.frameNo() == 0;

Nit: add {}

Comment 11

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

Comment on attachment 8956715 [details] [diff] [review]
Part 2: Move hasCachedSavedFrame flag access from FrameIter to LiveSavedFrameCache::FramePtr.

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

r=me with comments below, if possible.

::: js/src/vm/Stack-inl.h
@@ +1014,5 @@
>  }
>  
>  inline void
> +LiveSavedFrameCache::FramePtr::setHasCachedSavedFrame() {
> +    ptr.match(SetHasCachedMatcher());

I forget, can you define structs within a method in C++, such that they are only visible to that method? If so, then moving the matcher structs inside these methods would be a nice readability clean up, IMO.

Failing that, rather than defining the matchers together and then the methods together, it would be easier to read and understand a method if it was ordered like

  struct BlahMatcher { ... }
  inline bool blahMethod() { ... }

  struct DodoMatcher { ... }
  inline bool dodoMethod() { ... }

::: js/src/vm/Stack.h
@@ +1178,5 @@
> +
> +        Ptr ptr;
> +
> +        struct HasCachedMatcher;
> +        struct SetHasCachedMatcher;

As alluded to above, I don't think we are getting any value out of havign these classes be members of FramePtr, rather than simply an implementation detail.

Comment 12

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

Comment on attachment 8956716 [details] [diff] [review]
Part 3: Change LiveSavedFrameCache::find to take a FramePtr, not a FrameIter.

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

Nice.

Comment 13

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

Comment on attachment 8956718 [details] [diff] [review]
Part 5: Move LiveSavedFrameCache::getFramePtr to FramePtr::make.

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

::: js/src/vm/Stack-inl.h
@@ +998,5 @@
>      return true;
>  }
>  
> +/* static */ inline Maybe<LiveSavedFrameCache::FramePtr>
> +LiveSavedFrameCache::FramePtr::make(const FrameIter& iter)

Nitpick: most of our factory functions in SM are "create", so I think that would be a better choice than "make".

Comment 14

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

Comment on attachment 8956719 [details] [diff] [review]
Part 6: Remove hasCachedSavedFrame accessors from AbstractFramePtr.

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

::: js/src/vm/Stack.h
@@ +1180,3 @@
>  
>          Ptr ptr;
>          explicit FramePtr(Ptr ptr) : ptr(ptr) { }

I feel like there is an opportunity for some perfect forwarding of args to implicitly construct Ptr, rather than taking an already constructed Ptr, but it is up to you whether you think that is worth it or not.

Comment 15

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

This is a great clean up! Thanks :)

Comment 16

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> I forget, can you define structs within a method in C++, such that they are
> only visible to that method? If so, then moving the matcher structs inside
> these methods would be a nice readability clean up, IMO.

I thought you couldn't, but you can! And that way is much nicer.

Comment 17

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #16)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> > I forget, can you define structs within a method in C++, such that they are
> > only visible to that method? If so, then moving the matcher structs inside
> > these methods would be a nice readability clean up, IMO.
> 
> I thought you couldn't, but you can! And that way is much nicer.

Oh, no, local classes can't have member templates: http://en.cppreference.com/w/cpp/language/class#Local_classes

That breaks Part 6. I guess they'll have to go in a 'detail' namespace.

Comment 18

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #14)
> I feel like there is an opportunity for some perfect forwarding of args to
> implicitly construct Ptr, rather than taking an already constructed Ptr, but
> it is up to you whether you think that is worth it or not.

I changed it to:

        template<typename Frame>
        explicit FramePtr(Frame ptr) : ptr(ptr) { }

and then the calls are like:

        return mozilla::Some(FramePtr(iter.abstractFramePtr()));
        return mozilla::Some(FramePtr(iter.physicalIonFrame()));

I think this will just pass the AbstractFramePtr / CommonFrameLayout* / gefiltefish through and apply Ptr's constructor to that, which should construct it in place. Is that what you were looking for?

(It was a bug to pass Ptr by value at all... :( )

Comment 19

[Jim Blandy :jimb]

Try for revised patch series:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15c706ca4a7a9cabbcbde9c720a34a23b6c38a6

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/faa2dd792d1c
Part 1: Introduce opaque LiveSavedFrameCache::Key type for cache keys. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/1790490874a0
Part 2: Move hasCachedSavedFrame flag access from FrameIter to LiveSavedFrameCache::FramePtr. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/269e77f6b1e5
Part 3: Change LiveSavedFrameCache::find to take a FramePtr, not a FrameIter. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/3262afac47e8
Part 4: Enable saved frame caching for wasm debug frames. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8eff45d0955
Part 5: Move LiveSavedFrameCache::getFramePtr to FramePtr::create. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/797239d1e705
Part 6: Remove hasCachedSavedFrame accessors from AbstractFramePtr. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e897d02ea9
Part 7: Use jit::CommonFrameLayout to store the hasCachedSavedFrame flag for Baseline frames. r=jandem

Comment 21

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

(In reply to Jim Blandy :jimb from comment #18)
> Is that what you were looking for?

Yes, thanks!

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/faa2dd792d1c
https://hg.mozilla.org/mozilla-central/rev/1790490874a0
https://hg.mozilla.org/mozilla-central/rev/269e77f6b1e5
https://hg.mozilla.org/mozilla-central/rev/3262afac47e8
https://hg.mozilla.org/mozilla-central/rev/c8eff45d0955
https://hg.mozilla.org/mozilla-central/rev/797239d1e705
https://hg.mozilla.org/mozilla-central/rev/b4e897d02ea9