1384683 - Hide jit/wasm frame iterators behind an higher-level frame iterator

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

This is just a spin off of bug 1360211, to not spam too much there.

I've got a WIP patch (that I think is equivalent to current behavior, but that needs to be confirmed) but there are a few remaining things I'd like to address:

- find the right names for the new structures:
  - CodeSegmentIter -> FrameGroupIter
  - JitSegmentIter -> JitFrameGroupIter (or JSJitFrameGroupIter)
  - WasmSegmentIter -> WasmFrameGroupIter
- rename JitSegmentIter::jitFrame() to frame(), for simplicity and symmetry with the WasmSegmentIter.
- it appears most users of JitSegmentIter::jitFrame() (resp. WasmSegmentIter::frame()) can deal with a const& to the internal iterator. So I had this idea of splitting these internal frame iterators into two parts: a static JitFrameView class (that can be constant and returned by the frame() methods) and a JitFrameMutableView (that implements operator++/ctor/forwardLiveInstance), used only internally in the FrameGroupIter (instead of the deceptive friendship declarations).
- <Rust advertisement starting here> There's no way to prevent the outer iterator to escape scope while the inner iterators is still being used.</ad> Probably we could have a temporary scope object returned by the frame() methods, at least to prevent outliving issues.
- There's some funny business with the DebugModeOSRVolatileFrameIterator (and JitFrameIterator), as well as the wasm::FrameIterator::setUnwind() method, that need to be figured out. I feel these are two instances of the same design issue.
- There's a choice to be done in the FrameIter for what's the preferred way to deal with FrameGroupIter: externally use isJit()/isWasm()/asJit()/asWasm(), or have Match structures that hide the behavior inside FrameGroupIter methods. Right now, it's a mix of the two, since I wanted to try both approaches, but it's messy. When the JitFrameIterator needs the Ion inline frame iterator, we need to pass the latter to the FrameGroupIter, even though it's unused for wasm; so the internal Match structures don't seem to fit that usage well.

Other renamings we've discussed on IRC and that could happen here too:
- rename jit::JitFrameIterator -> js::JitFrameIter
- rename wasm::FrameIterator -> js::WasmFrameIter
(so it's consistent with js::FrameIter)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.framegroup.patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8891422 [details] [diff] [review]
1.framegroup.patch

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

Not entirely ready, but I'd appreciate if you could give it a first look please (jandem for overall mechanism, luke for wasm changes and consistency with plan), especially since I'll be away next week.

Notable changes are in:
- Stack.h/cpp, for the implementations of the FrameGroupIter.
- JitFrameIterator.h/cpp and WasmFrameIterator.h/cpp for the few changes there.
- the BaselineDebugModeOSR.h/cpp, for the specialization FrameGroupIter for BaselineDebugModeOSRFrameGroupIter.

Other files are mostly mechanical changes to use the FrameGroupIter instead of the JitFrameIterator.

Note that by API, users won't be able to incorrectly use JitFrameIterator/wasm::FrameIterator: its ctor, operator++ and done() methods are now private and accessible only to the friend FrameGroupIter class (more about that soon).

I just realized I'd have to do the same for the profiling frame iterator, since it implements operator++ as well.

This patch doesn't handle the case where an internal iterator (e.g. JitFrameIterator) escapes, and is used after the outer iterator (FrameGroupIter) is dead. I was thinking about using refcounting with RefCounted/RefPtr here, so that each FrameGroupIter::operator++() ensures there are no references to any internal iterator. Is there a simpler way?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.frameview.patch

This one implements one idea I had in the first comment: move all the const methods from JitFrameIterator into a JitFrameView class, and have the JitFrameIterator be internal to the FrameGroupIter, so it can't be misused and removes the friendship.

It makes enhances naming a lot, in my opinion. A frozen frame iterator is just a view on the frame.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8891422 [details] [diff] [review]
1.framegroup.patch

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

I just skimmed the patch, except for the Wasm* files, approach looks good to me :)

::: js/src/vm/Stack.h
@@ +1777,5 @@
> +// The specialized JitFrameGroupIter defined after this class can filter out all
> +// the wasm frames, if you're only interested in Jit frames.
> +//
> +// TODO(bug 1360211) In particular, this can handle the transition from wasm to
> +// ion and from ion to wasm, since these will be able to live under a same

Nit: "a same" -> "a single" or "the same"

@@ +1788,5 @@
> +// is allowed to call these.
> +class FrameGroupIter
> +{
> +    mozilla::Maybe<mozilla::Variant<jit::JitFrameIterator,
> +                                    wasm::FrameIterator>> iter_;

Nit: consider using mozilla::MaybeOneOf instead of Maybe<Variant<>>

@@ +1891,5 @@
> +
> +    enum State {
> +        DONE,      // when there are no more frames nor activations to unwind.
> +        INTERP,    // interpreter activation on the stack
> +        FRAMEGROUP // jit or wasm activations on the stack

FRAMEGROUP might be a bit confusing for people not familiar with the code. I wonder if we should just use JIT (similar to how we use JitActivation) or JIT_OR_WASM.

It would be really nice to have a good term for JIT-or-WASM, then we could use it everywhere - JitActivation/WasmActivation -> FooActivation etc :) I can live with FrameGroupIterator but it's a bit confusing sometimes.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8891424 [details] [diff] [review]
2.frameview.patch

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

Great idea! I like it.

::: js/src/vm/Stack.h
@@ +1788,5 @@
>  // is allowed to call these.
>  class FrameGroupIter
>  {
> +  protected:
> +    class JitFrameIterator : public jit::JitFrameView {

What if we make the iterator *contain* a JitFrameView, instead of inheriting from one? Would that complicate things?

@@ +1795,5 @@
> +
> +        // Functions used to iterate on frames. When prevType is JitFrame_Entry,
> +        // the current frame is the last frame.
> +        void operator++();
> +        bool done() const { return jit::JitFrameView::done(); }

Should we remove done() from JitFrameView, as it belongs to the iterator and not really to a single frame?

Comment 6

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> FRAMEGROUP might be a bit confusing for people not familiar with the code. I
> wonder if we should just use JIT (similar to how we use JitActivation) or
> JIT_OR_WASM.
> 
> It would be really nice to have a good term for JIT-or-WASM, then we could
> use it everywhere - JitActivation/WasmActivation -> FooActivation etc :) I
> can live with FrameGroupIterator but it's a bit confusing sometimes.

Agreed with Jan here.

Despite the fact that we're often taking "Jit" to mean "JS Jit", I really kindof like "Jit" meaning "compiled code, JS or wasm", and then we could use "JSJit" and "Wasm" to specify which kind of Jit.  (Open to bikeshed "Jit" vs. "JIT" etc.)

Moreover, over time, we may want to factor out a core JIT (=cretonne+masm) from JSJit (=IonBuilder,JS baseline) from Wasm (=Baldr,Rabaldr), so that "JIT" really is JS/wasm-independent.

WDYT Jan?

Comment 7

[Luke Wagner [:luke]]

A small variation on comment 6 that is more terse and symmetric with wasm is to use "JS" (or "Js") instead of "JSJit".  Given that most of these things would be inside jit:: this doesn't seem ambiguous.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8891422 [details] [diff] [review]
1.framegroup.patch

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

Great job, this looks like the right design.  r+ assuming we figure out the right names as discussed above.

::: js/src/jit/Ion.cpp
@@ +591,5 @@
>  jit::LazyLinkTopActivation()
>  {
>      // First frame should be an exit frame.
>      JSContext* cx = TlsContext.get();
> +    const JitFrameIterator& it = JitFrameGroupIter(cx).frame();

I think 'it' here will hold onto a reference to a field of a temporary JitFrameGroupIter object that is destroyed at the end of the statement.

::: js/src/vm/Stack.cpp
@@ +503,5 @@
> +    return false;
> +}
> +
> +void
> +FrameGroupIter::settleToScriptedFrame()

nit: Given the following definition and how it's used, I think a slightly more clear name would be skipNonScriptedJSFrames().

@@ +536,5 @@
> +void
> +FrameGroupIter::operator++()
> +{
> +    if (!iter_)
> +        return;

I think you should be able to MOZ_ASSERT(iter_) no?

@@ +548,5 @@
> +    }
> +    MOZ_CRASH("unhandled case");
> +}
> +
> +JitFrameGroupIter::JitFrameGroupIter(Activation* act) {

nit: { on new line here and below

@@ +1080,5 @@
>  
>              // Look for the current frame.
> +            data_.frameGroup_.reset();
> +            MOZ_ALWAYS_TRUE(data_.frameGroup_.tryConstructFrom(data_.activations_->asJit()));
> +            data_.frameGroup_.settleToScriptedFrame();

This settleToScriptedFrame() seems new and I wouldn't think necessary (given that we're basically skipping over everything but baseline frames == 'frame'.

::: js/src/vm/Stack.h
@@ +1777,5 @@
> +// The specialized JitFrameGroupIter defined after this class can filter out all
> +// the wasm frames, if you're only interested in Jit frames.
> +//
> +// TODO(bug 1360211) In particular, this can handle the transition from wasm to
> +// ion and from ion to wasm, since these will be able to live under a same

uber nit: s/will be able to live under/will be interleaved in/

@@ +1795,5 @@
> +    FrameGroupIter() : iter_(mozilla::Nothing()) {}
> +
> +    // Returns true iff the frame group iterator could be constructed from a
> +    // JIT or wasm activation.
> +    bool tryConstructFrom(Activation* activation);

If we can have Jit = JS or Wasm and when WasmActivation is killed, then I think this method could just be a constructor taking a JitActivation.  Until then (in this patch), I think you could still have a constructor taking an Activation* asserting isJit() || isWasm().

::: js/src/wasm/WasmBuiltins.cpp
@@ +103,5 @@
>      JSContext* cx = activation->cx();
>  
> +    // TODO(bug 1360211): when JitActivation and WasmActivation get merged,
> +    // we'll be able to switch to ion / other wasm state from here, and we'll
> +    // need to do things differently.

This function is only called directly from wasm so, iiuc, you should be able to unconditionally get a wasm iterator for the innermost frame.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #6)
> Despite the fact that we're often taking "Jit" to mean "JS Jit", I really
> kindof like "Jit" meaning "compiled code, JS or wasm", and then we could use
> "JSJit" and "Wasm" to specify which kind of Jit.  (Open to bikeshed "Jit"
> vs. "JIT" etc.)
> 
> Moreover, over time, we may want to factor out a core JIT (=cretonne+masm)
> from JSJit (=IonBuilder,JS baseline) from Wasm (=Baldr,Rabaldr), so that
> "JIT" really is JS/wasm-independent.
> 
> WDYT Jan?

Yes this sounds good to me. So we would have JitFrameIterator wrapping JSJitFrameIterator and WasmFrameIterator (or maybe s/Iterator/Iter/).

(In reply to Luke Wagner [:luke] from comment #7)
> A small variation on comment 6 that is more terse and symmetric with wasm is
> to use "JS" (or "Js") instead of "JSJit".  Given that most of these things
> would be inside jit:: this doesn't seem ambiguous.

I'd prefer |JSJit| over |JS| because JSFrameIter (for instance) suggests it will iterate over *all* JS frames. The jit:: namespace doesn't help much when reading code because a lot of files use |using namespace js::jit;|. JSJitFrameIter is still a pretty short name.

Benjamin, sorry for going back and forth on this :/

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the preliminary reviews! And no worries for the back and forth on naming, it's better to have good names from the start.

So in the transient patches, there'll be JitFrameIter (~ FrameGroupIter) as well as JitFrameIterator (the current one). It's confusing, but temporary: the second patch renames JitFrameIterator to LinearJSJitFrameIter, which is the iterator that can unwind a sequence of JIT frames that are *not* interleaved with frames of other types. I also thought of SequentialJSJitFrameIter, but not sure which is better. (Note that's the internal class, not used by anything else than JitFrameIter)

If that refactoring makes sense, I could do it also for wasm::FrameIterator, to be split into WasmFrameView (the const parts) / LinearWasmFrameIter (mutable operators). Does that make sense, Luke?

(In reply to Jan de Mooij [:jandem] from comment #4)
> @@ +1788,5 @@
> > +// is allowed to call these.
> > +class FrameGroupIter
> > +{
> > +    mozilla::Maybe<mozilla::Variant<jit::JitFrameIterator,
> > +                                    wasm::FrameIterator>> iter_;
> 
> Nit: consider using mozilla::MaybeOneOf instead of Maybe<Variant<>>

Thanks! I didn't know about MaybeOneOf. Unfortunately, in the first patch, it can't be done, because then the (current) JitFrameIterator class would need to be friend with mozilla::MaybeOneOf (since the latter calls new on the former). That's a very easy change I'll do as a follow-up, though, since everything is well encapsulated.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Summarizing my proposal above:

                 +---------------+      +---------------+
                 |               |      |               |
                 |               |      |               |
                 |JSJitFrameView |      |WasmFrameView  |
                 |  (immutable)  |      |  (immutable)  |
                 +------^--------+      +-------^-------+
                        |                       |
                        |        inherit        |
                        |                       |
                        |                       |
                 +------+--------+     +--------+------+
Currently        |               |     |               | Currently wasm::FrameIterator
JitFrameIterator |               |     |               |
                 |JSJitLinearIter|     |WasmLinearIter |
                 |   (mutable)   |     |    (mutable)  |
                 +------^--------+     +--------^------+
                        |                       |
                        |         has           |
                        |                       |
                        +---+--------------+----+
                            |              |
                            |              |
                            |JitFrameIter  |
                            |              |
                            +--------------+

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.rename-wasm-frameiter.patch

Renames wasm::FrameIterator -> wasm::WasmFrameIter.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.wip.patch

WIP patch for the new renamings. (Note to future self: don't ever think doing a renaming *and then* rebasing a gigantic patch over the renaming will be a good idea)

TODO:
- use MaybeOneOf in JitFrameIter, since we're fine with having public internal iterators ctors
- try luke's suggestion for JitFrameIter ctor
- more renamings need to happen here (JitFrameIterator -> JSJitFrameIter, maybe profiling iterators -> profiling iter)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d456d328c7
Rename wasm::FrameIterator into wasm::WasmFrameIter; r=luke

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.jitframeiter.patch

Changed to a MaybeOneOf, changed tryConstructFrom to a ctor, a few comments added in and there, a few renamings; try will confirm, but this is ready to re-review.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8897056 [details] [diff] [review]
2.jitframeiter.patch

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

::: js/src/jit/VMFunctions.cpp
@@ +364,5 @@
>      // AutoDetectInvalidation uses GetTopJitJSScript(cx)->ionScript(), but it's
>      // possible the SetOrExtendAnyBoxedOrUnboxedDenseElements call already
>      // invalidated the IonScript. JitFrameIterator::ionScript works when the
>      // script is invalidated so we use that instead.
> +    OnlyJSJitFrameIter it(cx);

I would think this could use a JSJitFrameIter directly.  (And below.)  In general, I was thinking using (JSJit|Wasm)FrameIter directly is useful for all these C++-stub cases where you are called directly from JSJit/wasm code and only care about the innermost frame since it asserts assumptions about the caller.  It'd be good to see which other uses of Only(JSJit|Wasm)FrameIter could lose the "Only".

::: js/src/vm/Stack.cpp
@@ +521,5 @@
> +    if (act->isJit()) {
> +        iter_.construct<jit::JitFrameIterator>(act->asJit());
> +    } else {
> +        MOZ_ASSERT(act->isWasm());
> +        iter_.construct<wasm::WasmFrameIter>(act->asWasm());

nit: for op= and this ctor, I think you can use the op= and ctor of MaybeOneOf directly.

::: js/src/vm/Stack.h
@@ +1780,5 @@
> +// - code generated for JS is referred to as JSJit.
> +// - code generated for wasm is referred to as Wasm.
> +// Also, Jit refers to any one of them.
> +//
> +// JitFrameIter uses JitFrameIterator to iterate over JSJit code or a

So, iiuc, jit::JitFrameIterator will be changed to jit::JSJitFrameIterator in the future?

@@ +2076,5 @@
>    private:
>      Data data_;
>      jit::InlineFrameIterator ionInlineFrames_;
>  
> +    // XXX a bit sad to need the const variants

nit: I'd remove this XXX comment since it doesn't seem to add much.

::: js/src/wasm/WasmBuiltins.cpp
@@ +102,5 @@
>      MOZ_ASSERT(activation);
>      JSContext* cx = activation->cx();
>  
> +    OnlyWasmFrameIter iter(activation);
> +    const WasmFrameIter& frame = iter.frame();

I think you should be able to construct a WasmFrameIter directly here (by invariant of we only call WasmHandleDebugTrap from wasm code).  And considering the other two uses of OnlyWasmFrameIter:
 1. for TraceActivations, it would be more efficient to merge wasm::TraceActivations with jit::TraceJitActivations so we do a single walk over the stack (branching per frame)
 2. for WasmHandleThrow, I think we'll only be able to unwind 1 wasm "frame group" at a time *anyway* (when the WasmFrameIter is done(), we'll need to return to the JSJit->Wasm entry stub to go back to plain JS unwinding), so it should use WasmFrameIter too
So can you can kill OnlyWasmFrameIter?

Based on the other comment, I'm wondering if even OnlyJSJitFrameIter is actually necessary; maybe it's actually a healthy discipline to be forced to consider "can a wasm frame appear here?" and, if so, "what does that mean?".  If the answer is "Yes" and "I ignore it" >50% of the time, then OnlyJSJitFrameIter makes sense, but otherwise maybe not...

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72d456d328c7

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8897056 [details] [diff] [review]
2.jitframeiter.patch

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

Looks good!

It's true that a lot of the OnlyJSJit uses are to look at the top frame (which must be JSJIt), so I agree it might be nice to use the JSJitFrameIter directly in these cases.

::: js/src/vm/Stack.cpp
@@ +773,5 @@
>  
>  void
>  FrameIter::popJitFrame()
>  {
> +    MOZ_ASSERT(data_.state_ == JIT && data_.jitFrames_.isSome());

Nit: I'd split in 2 separate asserts to make debugging easier when it fails.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #16)
> Comment on attachment 8897056 [details] [diff] [review]
> 2.jitframeiter.patch
> 
> Review of attachment 8897056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/VMFunctions.cpp
> @@ +364,5 @@
> >      // AutoDetectInvalidation uses GetTopJitJSScript(cx)->ionScript(), but it's
> >      // possible the SetOrExtendAnyBoxedOrUnboxedDenseElements call already
> >      // invalidated the IonScript. JitFrameIterator::ionScript works when the
> >      // script is invalidated so we use that instead.
> > +    OnlyJSJitFrameIter it(cx);
> 
> I would think this could use a JSJitFrameIter directly.  (And below.)  In
> general, I was thinking using (JSJit|Wasm)FrameIter directly is useful for
> all these C++-stub cases where you are called directly from JSJit/wasm code
> and only care about the innermost frame since it asserts assumptions about
> the caller.  It'd be good to see which other uses of
> Only(JSJit|Wasm)FrameIter could lose the "Only".

Makes sense, I'll check each of them.

> 
> ::: js/src/vm/Stack.cpp
> @@ +521,5 @@
> > +    if (act->isJit()) {
> > +        iter_.construct<jit::JitFrameIterator>(act->asJit());
> > +    } else {
> > +        MOZ_ASSERT(act->isWasm());
> > +        iter_.construct<wasm::WasmFrameIter>(act->asWasm());
> 
> nit: for op= and this ctor, I think you can use the op= and ctor of
> MaybeOneOf directly.

MaybeOneOf doesn't have a ctor that forwards arguments the constructed thing: it's the role of the construct<> method to do this. Also, MaybeOneOf::operator=(const MaybeOneOf&) is explicitly deleted. There's a move assignment operator, but using it is equivalent to having another temporary MaybeOneOf in scope and using the construct<> method, so not sure it is much better.

> 
> ::: js/src/vm/Stack.h
> @@ +1780,5 @@
> > +// - code generated for JS is referred to as JSJit.
> > +// - code generated for wasm is referred to as Wasm.
> > +// Also, Jit refers to any one of them.
> > +//
> > +// JitFrameIter uses JitFrameIterator to iterate over JSJit code or a
> 
> So, iiuc, jit::JitFrameIterator will be changed to jit::JSJitFrameIterator
> in the future?

Yes sir.

> 
> @@ +2076,5 @@
> >    private:
> >      Data data_;
> >      jit::InlineFrameIterator ionInlineFrames_;
> >  
> > +    // XXX a bit sad to need the const variants
> 
> nit: I'd remove this XXX comment since it doesn't seem to add much.

Oops, I thought I removed all these comments addressed to myself.

> 
> ::: js/src/wasm/WasmBuiltins.cpp
> @@ +102,5 @@
> >      MOZ_ASSERT(activation);
> >      JSContext* cx = activation->cx();
> >  
> > +    OnlyWasmFrameIter iter(activation);
> > +    const WasmFrameIter& frame = iter.frame();
> 
> I think you should be able to construct a WasmFrameIter directly here (by
> invariant of we only call WasmHandleDebugTrap from wasm code).  And
> considering the other two uses of OnlyWasmFrameIter:
>  1. for TraceActivations, it would be more efficient to merge
> wasm::TraceActivations with jit::TraceJitActivations so we do a single walk
> over the stack (branching per frame)
>  2. for WasmHandleThrow, I think we'll only be able to unwind 1 wasm "frame
> group" at a time *anyway* (when the WasmFrameIter is done(), we'll need to
> return to the JSJit->Wasm entry stub to go back to plain JS unwinding), so
> it should use WasmFrameIter too
> So can you can kill OnlyWasmFrameIter?

I'll look into this, thanks.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.jitframeiter.patch

(carrying forward r+es)

Updated patch:
- we could indeed get rid of OnlyWasmFrameIter, for the reasons that Luke mentioned. Thanks!
- for other uses of OnlyJSJitFrameIter, I've scanned each one of them and tried to figure out whether wasm frames could show up there. For some, it's very obvious and we only want the last frame, so I could revert to using a JitFrameIterator there (renaming pending). For others, it was unclear; I've kept JitFrameIterator where I was unsure, since it would probably cause assertions when it's incorrectly used.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.rename-jsjitframeiter.patch

This is the next renaming, long overdue: JitFrameIterator -> JSJitFrameIter.

As discussed on irc with Jan, we could also rename ProfilingFrameIterator to ProfilingFrameIter, for consistency. However, this class is exposed as part of the public JSAPI, so the renamings would spill over to gecko; plus it's nice that the public API has the full name, in my opinion.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8897984 [details] [diff] [review]
3.rename-jsjitframeiter.patch

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

LGTM!

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ba96d3eda8
Implement an higher-level frame iterator that can handle JS jit and wasm frames interleaving; r=jandem, r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab1901a6c35
Rename JitFrameIterator to JSJitFrameIter; r=jandem

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3ba96d3eda8
https://hg.mozilla.org/mozilla-central/rev/1ab1901a6c35