689556 - IonMonkey: Move IonFrameIterator class to IonFrame.cpp

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

IonFrame iterator is tied to the BailoutEnvironment.  Making the dependency weaker may allow to use the IonFrameIterator with either a BailoutEnvironment or a CCallEnvironment (known as IonCFrame in Bug 680315).

This issue is a blocker of Bug 680315 for a clean implementation of JS stack frame traversal made by C function called from the JS.

One possible direction is to make a base class for Environment which provide the feature expected by the IonFrameIterator and which are derived for the BailoutEnvironment and for CCallEnvironment.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

It is possible to use one structure for both, even if it doesn't directly map onto the native stack?

I.e. if we have something like:

  class IonState {
      uintptr_t *stack;
      double *floatRegs;
      uintptr_t *regs;

    public:
      ...
  };

And then construct that from either BailoutEnvironment and CCallEnvironment, and use *that* in IonFrameIterator instead (which should really be, IonFrameInspector or something).

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #1)
> It is possible to use one structure for both, even if it doesn't directly
> map onto the native stack?
> 
> I.e. if we have something like:
> 
>   class IonState {
>       uintptr_t *stack;
>       double *floatRegs;
>       uintptr_t *regs;
> 
>     public:
>       ...
>   };

This implies that you have to copy some from the BailoutStack and IonCStack (does not exists yet) to the IonFrameInspector class.  Thus the IonFrameInspector would become somekind of merge/union of the different environment.

I am not sure this is the cleanest thing to do but this can be done but I don't know the overhead due to mismatch between BailoutStack implementation on x86 and the IonCStack.  I have to check if this can be share.

> And then construct that from either BailoutEnvironment and CCallEnvironment,
> and use *that* in IonFrameIterator instead (which should really be,
> IonFrameInspector or something).

I agree with the renaming.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

> I am not sure this is the cleanest thing to do but this can be done but I
> don't know the overhead due to mismatch between BailoutStack implementation
> on x86 and the IonCStack.

Keep in mind this isn't the critical path; all the IonFrameIterator needs are pointers to the machine state.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: [0001] Snapshot do not interpret NULL start address as valid source.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: [0002] Rename IonFrameIterator into IonFrameInspector and use an abstract Environment.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 563063 [details] [diff] [review]
[0002] Rename IonFrameIterator into IonFrameInspector and use an abstract Environment.

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

::: js/src/ion/IonFramesAnalysis.cpp
@@ +66,5 @@
> +        return UndefinedValue();
> +    }
> +}
> +
> +uintptr_t IonFrameInspector::fromLocation(const SnapshotReader::Location &loc)

Nit: Return type on newline, i.e.:

uintptr_t
IonFrameInspector::fromLocation...

@@ +107,5 @@
> +        script = CalleeTokenToScript(frame->calleeToken());
> +    }
> +}
> +
> +Value IonFrameInspector::read()

Ditto

::: js/src/ion/IonFramesAnalysis.h
@@ +39,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef jsion_frames_analysis_h__
> +#define jsion_frames_analysis_h__

Can we put this in IonFrames.h/.cpp instead? Similar to vm/Stack.h it's nice to have both the stack layout and stack API in one place.

@@ +54,5 @@
> +    JSFunction *fun;
> +    JSObject *callee;
> +    JSScript *script;
> +
> +    ScriptInfo(IonFramePrefix *frame);

Instead of introducing this struct, could we add fun(), callee(), and script() accessors to IonFramePrefix?

@@ +90,5 @@
> +  public:
> +
> +    IonFrameInspector(IonEnvironment& env);
> +
> +    void scriptInfoInit(ScriptInfo&);

It looks like this doesn't get used.

::: js/src/ion/x64/Bailouts-x64.cpp
@@ +72,5 @@
> +        BailoutId id = bailoutId();
> +        return ionScript->bailoutToSnapshot(id);
> +    } else {
> +        return bailout_->snapshotOffset();
> +    }

x64 does not use bailout IDs, you should be able to assert that frameClass() == FrameSizeClass::None()

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 563061 [details] [diff] [review]
[0001] Snapshot do not interpret NULL start address as valid source.

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

::: js/src/ion/Snapshots.cpp
@@ +114,5 @@
>      , slotsRead_(0)
>  #endif
>  {
> +    if (!buffer)
> +        return;

This patch looks fine but when would we instantiate with a NULL buffer? If it's not allowed, assert is good enough.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #6)
> Comment on attachment 563063 [details] [diff] [review] [diff] [details] [review]
> [0002] Rename IonFrameIterator into IonFrameInspector and use an abstract
> Environment.
> 
> Review of attachment 563063 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonFramesAnalysis.h
> @@ +39,5 @@
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +#ifndef jsion_frames_analysis_h__
> > +#define jsion_frames_analysis_h__
> 
> Can we put this in IonFrames.h/.cpp instead? Similar to vm/Stack.h it's nice
> to have both the stack layout and stack API in one place.

The idea was to avoid including Snapshot.h which comes with all Assembly dependencies.

> @@ +54,5 @@
> > +    JSFunction *fun;
> > +    JSObject *callee;
> > +    JSScript *script;
> > +
> > +    ScriptInfo(IonFramePrefix *frame);
> 
> Instead of introducing this struct, could we add fun(), callee(), and
> script() accessors to IonFramePrefix?

Sure we can move it but I preferred to create this structure to avoid duplicating efforts and avoid adding memory which is not frequently read.  I did that to factor between the IonFrameInspector constructor and the Bailout.cpp:ConvertFrames function.

In fact I miss the possibility to initialize the IonFrameInspector with a ScriptInfo.

> @@ +90,5 @@
> > +  public:
> > +
> > +    IonFrameInspector(IonEnvironment& env);
> > +
> > +    void scriptInfoInit(ScriptInfo&);
> 
> It looks like this doesn't get used.

Hum .. neither defined apparently.

> ::: js/src/ion/x64/Bailouts-x64.cpp
> @@ +72,5 @@
> > +        BailoutId id = bailoutId();
> > +        return ionScript->bailoutToSnapshot(id);
> > +    } else {
> > +        return bailout_->snapshotOffset();
> > +    }
> 
> x64 does not use bailout IDs, you should be able to assert that frameClass()
> == FrameSizeClass::None()

Ok.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #7)
> Comment on attachment 563061 [details] [diff] [review] [diff] [details] [review]
> [0001] Snapshot do not interpret NULL start address as valid source.
> 
> Review of attachment 563061 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/Snapshots.cpp
> @@ +114,5 @@
> >      , slotsRead_(0)
> >  #endif
> >  {
> > +    if (!buffer)
> > +        return;
> 
> This patch looks fine but when would we instantiate with a NULL buffer? If
> it's not allowed, assert is good enough.

The IonFrameInspector constructor use this.

> +IonFrameInspector::IonFrameInspector(IonEnvironment& env)
> +  : ionScript_(0),
> +    env_(env),
> +    reader_(0, 0)
> +{
>      ...
> +    reader_ = SnapshotReader(start, end);
> +}

I would prefer to avoid allocation but the best would be to avoid the bad initialization too.  May be a placement new on a space reserved for the snapshot reader could help here.

class IonFrameInspector
{
    ...
    char[sizeof(SnapshotReader)] reader_;

    SnapshotReader&
    IonFrameInspector::reader()
    {
        return *reinterpret_cast<SnapshotReader*>(&reader_);
    }

    ...
};

IonFrameInspector::IonFrameInspector(IonEnvironment& env)
  : ionScript_(0),
    env_(env)
{
    ...
    new (reader_) SnapshotReader(start, end);
}

What do you think?

In which case we can replace the if(!buffer) by assertions.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

> The idea was to avoid including Snapshot.h which comes with all Assembly
> dependencies.

Taking in those dependencies is fine, at the least, preferable to splitting up tightly related functionality.

> Sure we can move it but I preferred to create this structure to avoid
> duplicating efforts and avoid adding memory which is not frequently read.  I
> did that to factor between the IonFrameInspector constructor and the
> Bailout.cpp:ConvertFrames function.

Sorry, I didn't mean adding callee/script members to IonFramePrefix, just accessors. Everything in ScriptInfo can be computed from the frame prefix, like:

    JSObject *callee() const {
        JS_ASSERT(IsCalleeTokenFunction(calleeToken());
        return CalleeTokenToFunction(calleeToken());
    }

This also makes it act more like a js::StackFrame.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

> The IonFrameInspector constructor use this.
> 
> > +IonFrameInspector::IonFrameInspector(IonEnvironment& env)
> > +  : ionScript_(0),
> > +    env_(env),
> > +    reader_(0, 0)
> > +{
> >      ...
> > +    reader_ = SnapshotReader(start, end);
> > +}


Ah, okay - thanks for explaining.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: [0002] Rename IonFrameIterator into IonFrameInspector and use an abstract Environment.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 567787 [details] [diff] [review]
[0002] Rename IonFrameIterator into IonFrameInspector and use an abstract Environment.

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

r=me with IonFrameAnalysis* merged into IonFrames* (analogous to vm/Stack.h, the data structures themselves are small, and it's nice to have all of the logic in one place).

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Rename IonFrameIterator into IonFrameInspector and use an abstract Environment.

- Move modifications to IonFrames.{h,cpp}
- Extract enum type definition from Bailouts.h to avoid circular dependencies of headers.
- Use Maybe<T> instead of a hack inside the SnapshotReader.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sorry, I think bug 695897 greatly bitrotted this patch. Most of the Bailout-$(ARCH) code has been removed for the much-simpler FrameRecovery/IonFrameIterator abstraction. Moving the inspector to IonFrames.* is still a good idea though, and I think that part of this patch should still work.

Chris Leary is interested in starting on-stack invalidation soon, which I think would be the first non-bailout consumer of IonFrameInspector - he should weigh in on what we might need.

Comment 16

[Nicolas B. Pierron [:nbp]]

These modifications are now part of the frameInfo patch (see Bug 695887) which is rebase on top of the recent modifications.