1073033 - Scalar Replacement of Objects should handle scope chains.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

At the moment, if we inline all inner functions, we still have to allocate a scope chain in case of bailouts.  This implies that we have to maintain a valid state, by allocating the scope object and storing all mutations back on the scope chain.

1/ To remove this, we have to ensure that the scope chain can be recovered on bailout, which means that even if it is still observable, we need to be able to recover its content.

2/ We need to add a logic to determine if the scope chain is escaped through an inner function.

3/ Replace all modification of the scope chain made from the outermost function or from the function environment of the inner functions.

4/ Recover the scope chain allocation.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: forEach-like micro benchmark.

Without the any modifications:
  for-loop: 0.8728031005859375 ns
  forEach: 2.4669666015625 ns

With the modifications for this bug and Bug 1087468:
  for-loop: 0.873868017578125 ns
  forEach: 1.04043779296875 ns

Currently there is still one thing which prevent IonMonkey to optimize Array.prototype.forEach, and I will work on it next.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Enable recovering the scope chain. r=shu

This patch use maybeRead for reading the scope chain, and it update
signatures of functions in consequences.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.0 - Snapshot: Add Recover instruction with default value. r=

This patch add a a new RValueAllocation, such as we can recover a default
value which is good enough for most of the use cases, when we do not want to
invalidate the script by running the recover instructions.

The only use case of this function so far is for findNextFrame, which record
the callee to read things such as the number of arguments.  The other
patches "part 2.*" are here to update everything to support the default
value which are read from the snapshot.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.1 - InlineFrameIterator: Recover the non-default value of a function. r=

This patch update InlineFrameIterator to use the default value by default
and recover the real value on demand.  The follow patches "part 2.*" are
updating the call-site accordingly.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.2 - Update callee uses, extract information form the calleeTemplate.

Update call-site which can be satisfied by reading the template instead of
having the real callee.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.3 - Update callee uses, extract information form the maybe-recovered callee.

Update call-site which cannot be satisfied by the template, and as such
might cause an invalidation of the IonCode if we have to recover the value
of the lambda.

The |FrameIter::callee(cx)| code path is checked by the
return_f.caller.caller test case of "part 3". Other code path are hard
(impossible?) to check while causing an invalidation.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.4 - ScopeChain & Callee: Remove new rooting hazards.

Removing rooting hazards from the previous patches.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Recover MLambda on bailouts.

This patch is like any other allocation removal with recover instruction
except that it provides a default value which is used by findNextFrame, to
read the calleeTemplate_.

With MFunctionEnvironment::foldsTo, this remove the allocation of lambdas,
even if we are unable to emulate the state of the CallObject.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Emulate states of NewCallObject.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8512794 [details] [diff] [review]
part 4 - Emulate states of NewCallObject.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +199,5 @@
>                  JitSpewDef(JitSpew_Escape, "Object ", ins);
>                  JitSpewDef(JitSpew_Escape, "  has a non-matching guard shape\n", guard);
>                  return true;
>              }
> +            if (IsObjectEscaped(def->toInstruction(), obj))

IIUC this is a pre-existing bug. Lets create another bug for this and land this separately. So we don't backout this fix by accident if this patch or other parts have some issues.

@@ +204,5 @@
>                  return true;
>              break;
>            }
> +
> +          case MDefinition::Op_Lambda: {

This iteration of uses looks a lot like the iteration in IsObjectEscaped itself.
Would it be possible to do?:

return IsObjectEscaped(Lambda);

and support Lambda in IsObjectEscaped. By adding case MDefinition::Op_FunctionEnvironment here?

@@ +519,5 @@
> +ObjectMemoryView::visitFunctionEnvironment(MFunctionEnvironment *ins)
> +{
> +    // Skip construction made with other objects.
> +    if (ins->foldsTo(alloc_) != obj_)
> +        return true;

foldsTo can have side-effects. It can create and/or MIRs and also add MIRs.
So we shouldn't call this function here.

What about adding a function to MFunctionEnvironment that has the logic of foldsTo. Which is used here and which MFunctionEnvironment uses in the call to foldsTo? That way we are certain this function doesn't alter the state, like foldsTo might do.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #10)
> ::: js/src/jit/ScalarReplacement.cpp
> @@ +199,5 @@
> >                  JitSpewDef(JitSpew_Escape, "Object ", ins);
> >                  JitSpewDef(JitSpew_Escape, "  has a non-matching guard shape\n", guard);
> >                  return true;
> >              }
> > +            if (IsObjectEscaped(def->toInstruction(), obj))
> 
> IIUC this is a pre-existing bug. Lets create another bug for this and land
> this separately. So we don't backout this fix by accident if this patch or
> other parts have some issues.

This is not a bug as there is no guard shape of a guard shape.  So this case cannot happen for the moment, but as we are adding this new argument, why not making right instead of giving nullptr?

> @@ +204,5 @@
> >                  return true;
> >              break;
> >            }
> > +
> > +          case MDefinition::Op_Lambda: {
> 
> This iteration of uses looks a lot like the iteration in IsObjectEscaped
> itself.
> Would it be possible to do?:
> 
> return IsObjectEscaped(Lambda);
> 
> and support Lambda in IsObjectEscaped. By adding case
> MDefinition::Op_FunctionEnvironment here?

This would be a terrible idea, because this would accept setting properties on the lambda, while we do not emulate the state of the MLambda.

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> (In reply to Hannes Verschore [:h4writer] from comment #10)
> > ::: js/src/jit/ScalarReplacement.cpp
> > @@ +199,5 @@
> > >                  JitSpewDef(JitSpew_Escape, "Object ", ins);
> > >                  JitSpewDef(JitSpew_Escape, "  has a non-matching guard shape\n", guard);
> > >                  return true;
> > >              }
> > > +            if (IsObjectEscaped(def->toInstruction(), obj))
> > 
> > IIUC this is a pre-existing bug. Lets create another bug for this and land
> > this separately. So we don't backout this fix by accident if this patch or
> > other parts have some issues.
> 
> This is not a bug as there is no guard shape of a guard shape.  So this case
> cannot happen for the moment, but as we are adding this new argument, why
> not making right instead of giving nullptr?

I'm not saying you need to give a nullptr. I was just suggesting this is actually not part of the patch "Emulate states of NewCallObject.". But actually fixes another problem, which also would have to get addressed, even if this patch didn't land. That's why I suggested another bug for it, which would get an immediate r+ from me.

> 
> > @@ +204,5 @@
> > >                  return true;
> > >              break;
> > >            }
> > > +
> > > +          case MDefinition::Op_Lambda: {
> > 
> > This iteration of uses looks a lot like the iteration in IsObjectEscaped
> > itself.
> > Would it be possible to do?:
> > 
> > return IsObjectEscaped(Lambda);
> > 
> > and support Lambda in IsObjectEscaped. By adding case
> > MDefinition::Op_FunctionEnvironment here?
> 
> This would be a terrible idea, because this would accept setting properties
> on the lambda, while we do not emulate the state of the MLambda.

ok.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8512772 [details] [diff] [review]
part 2.0 - Snapshot: Add Recover instruction with default value. r=

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

r+ if you agree with the {Main,Default}Value renaming, the use of readWithDefault in this patch, and you show me one way to ensure the assumption while tracing.

::: js/src/jit/IonFrames.cpp
@@ +1686,5 @@
>  
>        case RValueAllocation::RECOVER_INSTRUCTION:
>          return hasInstructionResult(alloc.index());
> +      case RValueAllocation::RI_WITH_DEFAULT_CST:
> +        return hasInstructionResult(alloc.index()) || rm == RM_DefaultValue;

I'd invert the tests, as the second one is cheaper to perform.

@@ +1874,5 @@
>          MOZ_CRASH("Recover instructions are handled by the JitActivation.");
>          break;
>  
> +      case RValueAllocation::RI_WITH_DEFAULT_CST:
> +        // Assume that we are always going to be writting on the default value

nit: writing

@@ +1875,5 @@
>          break;
>  
> +      case RValueAllocation::RI_WITH_DEFAULT_CST:
> +        // Assume that we are always going to be writting on the default value
> +        // while tracing.

Can you assert this in any way?

@@ +2237,5 @@
>          for (unsigned j = 0; j < skipCount; j++)
>              si_.skip();
>  
>          // The JSFunction is a constant, otherwise we would not have inlined it.
> +        Value funval = si_.readWithDefault();

Despite the comment, this seems wrong, as there is no user of the RecoverInstruction ctor with 2 uint32_t indexes in this patch (thus no RValueAllocation with the RI_WITH_DEFAULT_CST mode), unless I am missing something. Should this be in a later patch?

::: js/src/jit/JitFrameIterator.h
@@ +360,5 @@
>      RInstructionResults *instructionResults_;
>  
> +    enum ReadMethod {
> +        RM_MainValue,
> +        RM_DefaultValue,

Naming doesn't sound really appropriate here. How main is the MainValue? How about "Normal" for MainValue and "ReadOrDefault" for DefaultValue, or anything better you can think of?

@@ +489,5 @@
>      Value read() {
>          return allocationValue(readAllocation());
>      }
>  
> +    // Read the value by default, but if it is not available and that the

Hehe, the use of "default" here is tricky: by giving this a quick glance, one could read "default value". How about "Try to read the value, but if it is not available and the snapshot provides a default value" etc.?

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8512767 [details] [diff] [review]
part 1 - Enable recovering the scope chain. r=shu

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

I'm not that familiar with the scalar replacement code, and before I give r+ I'd like to make sure I understand how CallObjects are going to be recovered:

- Only MNewCallObject scope chains are subject to scalar replacement. Singleton CallObjects still get allocated.
- Non-escaping MNewCallObjects go through scalar replacement. An MObjectState encoding the state of the CallObject replaces the MNewCallObject in resume points.
- The MObjectState holds all the slots that the CallObject would have had alive in the resume point.
- MObjectState is recoverable.

Question:

- I see scalar replacement currently works on MNewObject and MCreateThisWithTemplate, both of which are canRecoverOnBailout. Where is the patch that makes MNewCallObject canRecoverOnBailout? How does the actual CallObject get allocated on recovery?

::: js/src/jit/CompileInfo.h
@@ +495,5 @@
>          // recover it.
>          if (!funMaybeLazy())
>              return true;
>  
> +        // The |this| and the |scopeChain| can be recovered.

Nit: add the word 'values' after |scopeChain|

::: js/src/jit/JitFrameIterator.h
@@ +723,4 @@
>          SnapshotIterator s(si_);
>  
>          // scopeChain
> +        Value v = s.maybeRead(fallback);

What happens on OOM here?

If we get an undefined, computeScopeChain is going to incorrectly report callee()->environment() or the global as the scope chain.

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8512776 [details] [diff] [review]
part 2.1 - InlineFrameIterator: Recover the non-default value of a function. r=

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

::: js/src/jit/IonFrames.cpp
@@ +1817,5 @@
> +        return allocationValue(a);
> +
> +    if (fallback.canRecoverResults()) {
> +        if (!initInstructionResults(fallback))
> +            return fallback.unreadablePlaceholder();

General question about error conditions here. On OOM, if the unreadablePlaceholder doesn't distinguish error from !fallback.canRecoverResults, who guarantees that OOMs don't result in incorrect behavior?

@@ +2214,5 @@
>  
>      si_ = start_;
>  
>      // Read the initial frame out of the C stack.
> +    calleeTemplate_ = frame_->maybeCallee();

This is never actually the template, right? This is always the actual callee.

@@ +2303,5 @@
> +
> +    // Handle non-recovering fallbacks the best that we can by returning the
> +    // template, if the unreadable value is returned.
> +    if (funval == fallback.unreadablePlaceholder())
> +        return calleeTemplate_;

How do the callers of InlineFrameIterator::callee() ensure correctness, if failure is silently falling back to calleeTemplate_?

::: js/src/jit/JitFrameIterator.h
@@ +589,5 @@
>      // frames contained in the recover buffer.
>      uint32_t frameCount_;
>  
> +    RootedFunction calleeTemplate_;
> +    RValueAllocation calleeRVA_;

The naming here kind of confuses me. calleeTemplate_ is only the template if it is a to-be-recovered lambda in an inline frame. Otherwise, it is the actual callee.

Similarly, calleeRVA_ is INVALID iff we were able to read out the callee, and non-INVALID if we need to get the default value.

Is that right?

@@ +619,1 @@
>      }

Add a comment here calleeTemplate() being the actual callee on non-inline frames.

I wish there was a better name for this, but short of pulling out all the stable metadata (nformals, etc) into another struct, I can't think of a better one.

::: js/src/vm/Stack.cpp
@@ +1122,5 @@
> +    // or the function from which the current function got cloned. When a
> +    // function is cloned via |CloneFunctionObjectIfNotSingleton|, we keep the
> +    // same displayAtom.
> +    if (calleeTemplate()->displayAtom() != fun->displayAtom())
> +        return false;

This line is iffy to me since I don't like heuristics depending on a displayed name, but I guess correctness isn't dependent on the displayAtom.

What do you think about the following? I don't think the speed of this function matters so much; it looks like it's only used in cases that are frowned upon, like getting arguments.caller.

if (!fun->isLambda())
    return calleeTemplate() == fun;

// Same logic as in js::CloneFunctionObject.
bool checkSameScript = calleeTemplate()->compartment() == fun->compartment() &&
                       !calleeTemplate()->hasSingletonType() &&
                       !types::UseNewTypeForClone(calleeTemplate());
if (checkSameScript && calleeTemplate()->nonLazyScript() != fun->nonLazyScript())
    return false;

return callee(cx) == fun;

::: js/src/vm/Stack.h
@@ +1614,5 @@
>      jsbytecode *pc() const { MOZ_ASSERT(!done()); return data_.pc_; }
>      void        updatePcQuadratic();
> +
> +    // The function |calleeTemplate()| returns either the function from which
> +    // the current |callee| was cloned ot the |callee| if it can be read. As

typo: or

Comment 16

[Shu-yu Guo [:shu]]

Comment on attachment 8512779 [details] [diff] [review]
part 2.2 - Update callee uses, extract information form the calleeTemplate.

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

::: js/src/jsopcode.cpp
@@ +1889,1 @@
>                         : nullptr);

Existing nit: line up the ternary operator after |cx,|

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8512787 [details] [diff] [review]
part 2.4 - ScopeChain & Callee: Remove new rooting hazards.

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

r=me with the readScopeChain and readReturnValue stuff inlined.

::: js/src/jit/JitFrameIterator.h
@@ +658,5 @@
> +        } else {
> +            s.readScopeChain(nullptr, fallback);
> +        }
> +
> +        s.readReturnValue(rval);

I'd prefer this not to be refactored to readScopeChain and readReturnValue, which do not guarantee reading the scope chain or the return value as it's dependent on the position of what allocations we've read so far. Plus, a method named readScopeChain that asserts that the fallback cannot GC is kind of strange.

If you want to remove readCommonFrameSlots, could you just inline it here and not add the extra helper methods?

Comment 18

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #14)
> I'm not that familiar with the scalar replacement code, and before I give r+
> I'd like to make sure I understand how CallObjects are going to be recovered:
> 
> - Only MNewCallObject scope chains are subject to scalar replacement.
> Singleton CallObjects still get allocated.

Yes, unfortunately the way Scalar Replacement works implies knowing everything about the object, which means from its allocation to its death.

> - Non-escaping MNewCallObjects go through scalar replacement. An
> MObjectState encoding the state of the CallObject replaces the
> MNewCallObject in resume points.
> - The MObjectState holds all the slots that the CallObject would have had
> alive in the resume point.
> - MObjectState is recoverable.

Yes, this is used when we are reconstructing the stack frame.  The MObjectState is recovered on bailout, and when we reconstruct the frame the Object state initialize all the properties of the object.

> 
> Question:
> 
> - I see scalar replacement currently works on MNewObject and
> MCreateThisWithTemplate, both of which are canRecoverOnBailout. Where is the
> patch that makes MNewCallObject canRecoverOnBailout? How does the actual
> CallObject get allocated on recovery?

It is not yet implemented, so, yes, we are still allocating a new call object without storing anything on it.  The content of the allocation is managed by Scalar Replacement, but the allocation does not have to be handled by the Scalar Replacement phase.  DCE is in charge of removing the allocation since there is no more live uses of it.

> ::: js/src/jit/JitFrameIterator.h
> @@ +723,4 @@
> >          SnapshotIterator s(si_);
> >  
> >          // scopeChain
> > +        Value v = s.maybeRead(fallback);
> 
> What happens on OOM here?
> 
> If we get an undefined, computeScopeChain is going to incorrectly report
> callee()->environment() or the global as the scope chain.

Right, I do not have any good alternative against that so far.  Not all of these paths were designed with the idea of becoming fallible.  So, having a near-OOM differential behaviour, while making it safe-ish sounds like an acceptable alternative.  Maybe I should always return the global and add additional spew here, such as we can easily identify such cases.

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #15)
> part 2.1 - InlineFrameIterator: Recover the non-default value of a function.
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonFrames.cpp
> @@ +1817,5 @@
> > +        return allocationValue(a);
> > +
> > +    if (fallback.canRecoverResults()) {
> > +        if (!initInstructionResults(fallback))
> > +            return fallback.unreadablePlaceholder();
> 
> General question about error conditions here. On OOM, if the
> unreadablePlaceholder doesn't distinguish error from
> !fallback.canRecoverResults, who guarantees that OOMs don't result in
> incorrect behavior?

Nothing. :(
Same story, we always assumed that these paths were infallible.  We need a way to make the OOM bubnle up to all callers.

> @@ +2214,5 @@
> >  
> >      si_ = start_;
> >  
> >      // Read the initial frame out of the C stack.
> > +    calleeTemplate_ = frame_->maybeCallee();
> 
> This is never actually the template, right? This is always the actual callee.

Right. In such case we get the callee from the stack, so the caller must have it push there before.

> @@ +2303,5 @@
> > +
> > +    // Handle non-recovering fallbacks the best that we can by returning the
> > +    // template, if the unreadable value is returned.
> > +    if (funval == fallback.unreadablePlaceholder())
> > +        return calleeTemplate_;
> 
> How do the callers of InlineFrameIterator::callee() ensure correctness, if
> failure is silently falling back to calleeTemplate_?

We don't same reason as before.

> ::: js/src/jit/JitFrameIterator.h
> @@ +589,5 @@
> >      // frames contained in the recover buffer.
> >      uint32_t frameCount_;
> >  
> > +    RootedFunction calleeTemplate_;
> > +    RValueAllocation calleeRVA_;
> 
> The naming here kind of confuses me. calleeTemplate_ is only the template if
> it is a to-be-recovered lambda in an inline frame. Otherwise, it is the
> actual callee.
> 
> Similarly, calleeRVA_ is INVALID iff we were able to read out the callee,
> and non-INVALID if we need to get the default value.
> 
> Is that right?

This is correct.

> @@ +619,1 @@
> >      }
> 
> Add a comment here calleeTemplate() being the actual callee on non-inline
> frames.
> 
> I wish there was a better name for this, but short of pulling out all the
> stable metadata (nformals, etc) into another struct, I can't think of a
> better one.

Having a struct which holds metadata which are stable across clones would be awesome for this use case.

> ::: js/src/vm/Stack.cpp
> @@ +1122,5 @@
> > +    // or the function from which the current function got cloned. When a
> > +    // function is cloned via |CloneFunctionObjectIfNotSingleton|, we keep the
> > +    // same displayAtom.
> > +    if (calleeTemplate()->displayAtom() != fun->displayAtom())
> > +        return false;
> 
> This line is iffy to me since I don't like heuristics depending on a
> displayed name, but I guess correctness isn't dependent on the displayAtom.
> 
> What do you think about the following? I don't think the speed of this
> function matters so much; it looks like it's only used in cases that are
> frowned upon, like getting arguments.caller.
> 
> if (!fun->isLambda())
>     return calleeTemplate() == fun;
> 
> // Same logic as in js::CloneFunctionObject.
> bool checkSameScript = calleeTemplate()->compartment() == fun->compartment()
> &&
>                        !calleeTemplate()->hasSingletonType() &&
>                        !types::UseNewTypeForClone(calleeTemplate());
> if (checkSameScript && calleeTemplate()->nonLazyScript() !=
> fun->nonLazyScript())
>     return false;
> 
> return callee(cx) == fun;

This sounds better than checking the displayAtom, as long as these fields remain immutable. :)

Comment 20

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> 
> It is not yet implemented, so, yes, we are still allocating a new call
> object without storing anything on it.  The content of the allocation is
> managed by Scalar Replacement, but the allocation does not have to be
> handled by the Scalar Replacement phase.  DCE is in charge of removing the
> allocation since there is no more live uses of it.
> 

Bear with me, I'm still confused.

Why is it safe for DCE to remove the allocation if MNewCallObject isn't recoverable yet? Where does the CallObject come from on bailout?

Is what you're saying that if nothing holds MNewCallObject live, *including* resume points, DCE can eliminate it. In that case, does the code in part 1 get exercised? Or is it setting up for the future, when MNewCallObject becomes recoverable?

Comment 21

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #20)
> (In reply to Nicolas B. Pierron [:nbp] from comment #18)
> > 
> > It is not yet implemented, so, yes, we are still allocating a new call
> > object without storing anything on it.  The content of the allocation is
> > managed by Scalar Replacement, but the allocation does not have to be
> > handled by the Scalar Replacement phase.  DCE is in charge of removing the
> > allocation since there is no more live uses of it.
> > 
> 
> Bear with me, I'm still confused.
> 
> Why is it safe for DCE to remove the allocation if MNewCallObject isn't
> recoverable yet? Where does the CallObject come from on bailout?

As it is not marked as recoverable "yet", and as such it would not be DCE-d, as it is still used by the MObjectState which are captured in the resume points.  This just means that Ion is still doing the allocation of the CallObject at the moment.

> Is what you're saying that if nothing holds MNewCallObject live, *including*
> resume points, DCE can eliminate it. In that case, does the code in part 1
> get exercised? Or is it setting up for the future, when MNewCallObject
> becomes recoverable?

Nothing prevent MNewCallObject to become recoverable, except the laziness of making the patch knowing that others are not r+ yet ;)

If no resume point capture any allocation, Ion should already be able to remove it, and if the only uses are resume point or instructions which are recovered on bailout, then it would be marked as recovered on bailout if possible.  Allocation have no observable side-effect based on the JavaScript semantic.  The only noticeable effect of removing allocations is the lack of GC.

Comment 22

[Shu-yu Guo [:shu]]

Nicolas, what do you think of hard crashing on OOM during recovery? It should be rare enough, right?

Comment 23

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #22)
> Nicolas, what do you think of hard crashing on OOM during recovery? It
> should be rare enough, right?

I think this might be a way to approach the problem on all paths where the oom is not checked.  This way we can migrate progressively from the infallible one to a fallible variant.

Jandem suggested to use CrashAtUnhandlableOOM, under the condition that these are small allocations, which they are.

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Comment on attachment 8512772 [details] [diff] [review]
> part 2.0 - Snapshot: Add Recover instruction with default value. r=
> 
> ::: js/src/jit/JitFrameIterator.h
> @@ +360,5 @@
> >      RInstructionResults *instructionResults_;
> >  
> > +    enum ReadMethod {
> > +        RM_MainValue,
> > +        RM_DefaultValue,
> 
> Naming doesn't sound really appropriate here. How main is the MainValue? How
> about "Normal" for MainValue and "ReadOrDefault" for DefaultValue, or
> anything better you can think of?

I changed this enum to have

  RM_Normal
  RM_Default
  RM_NormalOrDefault

Such that …

> @@ +1875,5 @@
> >          break;
> >  
> > +      case RValueAllocation::RI_WITH_DEFAULT_CST:
> > +        // Assume that we are always going to be writting on the default value
> > +        // while tracing.
> 
> Can you assert this in any way?

… the traceAllocation code can always use RM_Default, as opposed to RM_NormalOrDefault as it used to be.  Which ensure that this comment is correct.

> @@ +2237,5 @@
> >          for (unsigned j = 0; j < skipCount; j++)
> >              si_.skip();
> >  
> >          // The JSFunction is a constant, otherwise we would not have inlined it.
> > +        Value funval = si_.readWithDefault();
> 
> Despite the comment, this seems wrong, as there is no user of the
> RecoverInstruction ctor with 2 uint32_t indexes in this patch (thus no
> RValueAllocation with the RI_WITH_DEFAULT_CST mode), unless I am missing
> something. Should this be in a later patch?

Yes, this is done in the Lambda patch (part 3).

Do you want me to re-submit a patch for review, or can I carry on with the r+?

Comment 25

[Nicolas B. Pierron [:nbp]]

Attachment: part 2.1 - InlineFrameIterator: Recover the non-default value of a function.

Comment 26

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Emulate states of NewCallObject.

Comment 27

[Shu-yu Guo [:shu]]

Comment on attachment 8519075 [details] [diff] [review]
part 2.1 - InlineFrameIterator: Recover the non-default value of a function.

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

Please fix the OOM sites with crashes in part 1 as well.

Thanks!

::: js/src/jsfun.h
@@ +56,5 @@
>          ASMJS_LAMBDA_CTOR = ASMJS | NATIVE_CTOR | LAMBDA,
>          INTERPRETED_LAMBDA = INTERPRETED | LAMBDA,
> +        INTERPRETED_LAMBDA_ARROW = INTERPRETED | LAMBDA | ARROW,
> +        STABLE_ACROSS_CLONES = NATIVE_CTOR | IS_FUN_PROTO | EXPR_CLOSURE | HAS_GUESSED_ATOM |
> +                               LAMBDA | SELF_HOSTED | SELF_HOSTED_CTOR | HAS_REST | ASMJS | ARROW

Nice.

::: js/src/vm/Stack.cpp
@@ +1140,5 @@
> +
> +    // If none of the previous filtered worked, then take the risk of
> +    // invalidating the frame to identify the JSFunction.
> +    return callee(cx) == fun;
> +}

Great comments!

Comment 28

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #27)
> Comment on attachment 8519075 [details] [diff] [review]
> part 2.1 - InlineFrameIterator: Recover the non-default value of a function.
> 
> Review of attachment 8519075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please fix the OOM sites with crashes in part 1 as well.

I double checked. This is fixed locally ;)

Comment 29

[Hannes Verschore [:h4writer]]

Comment on attachment 8519076 [details] [diff] [review]
part 4 - Emulate states of NewCallObject.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +1010,5 @@
>  
>          for (MInstructionIterator ins = block->begin(); ins != block->end(); ins++) {
> +            if ((ins->isNewObject() || ins->isCreateThisWithTemplate() || ins->isNewCallObject()) &&
> +                !IsObjectEscaped(*ins))
> +            {

Can we adjust this to:
if (!isObjectEscaped(*ins)) {

and do the checking of isNewObject, isCreateThisWithTemplate ... into IsObjectEscaped?

Same with isArrayEscaped

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> I changed this enum to have
> 
>   RM_Normal
>   RM_Default
>   RM_NormalOrDefault
> 
> Such that the traceAllocation code can always use RM_Default, as opposed to
> RM_NormalOrDefault as it used to be.  Which ensure that this comment is
> correct.
> Do you want me to re-submit a patch for review, or can I carry on with the
> r+?

Okay to carry on the r+. Can you rename RM_Default into RM_AlwaysDefault or RM_OnlyDefault or RM_EnsureDefault in this case, please?

Comment 31

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #29)
> Comment on attachment 8519076 [details] [diff] [review]
> part 4 - Emulate states of NewCallObject.
> 
> Review of attachment 8519076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/ScalarReplacement.cpp
> @@ +1010,5 @@
> >  
> >          for (MInstructionIterator ins = block->begin(); ins != block->end(); ins++) {
> > +            if ((ins->isNewObject() || ins->isCreateThisWithTemplate() || ins->isNewCallObject()) &&
> > +                !IsObjectEscaped(*ins))
> > +            {
> 
> Can we adjust this to:
> if (!isObjectEscaped(*ins)) {
> 
> and do the checking of isNewObject, isCreateThisWithTemplate ... into
> IsObjectEscaped?

Unless we add a IsObjectEscaped_inner, this would not be a good idea.  For example, we want to check if the FunctionEnvironment is escaped or not, but this is unsafe to do it if this is a standalone instruction which cannot be replaced by the equivalent NewCallObject.  This is unsafe because the ScalarReplacement phase implies having full knowledge of the object and that it is not/ was not escaped.

This is a restriction that I want to tackle later this week, but not with this optimization phase.

Comment 32

[Nicolas B. Pierron [:nbp]]

Attachment: Add forEach micro benchmarks to the assorted test suite.

Comment 33

[Nicolas B. Pierron [:nbp]]

Note, even if these patches are green on try [1], I am not yet landing these patches because one the micro benchmark I made for AWFY (misc-forEach-custom.js) fails to return the expected result.  I guess this might be an issue related to the transition from int maths to double maths.

[1] https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a35b9aead76f

Comment 34

[Nicolas B. Pierron [:nbp]]

After some investigation, it appears that the problem is related to the resume points, and that for some reason we bailout under the forEach function.  The problem is that the forEach function does not capture directly the scope chain used by the function in which it is inlined and it only captures it via the outer-resume point of the block in which the forEach function is inlined.

Next to this issue, the lambda inserts a Phi node which inside the forEach function and this Phi node is captured by an object state.  This object state is only captured by the lambda, and only added to the resume point of the inlined lambda.

To solve this problem, we need a way to register the mutation of the state of an object which is indirectly captured by a resume point.  The proper way to fix this issue is to fix Bug 991720.

Comment 35

[Hannes Verschore [:h4writer]]

Comment on attachment 8522297 [details] [review]
Add forEach micro benchmarks to the assorted test suite.

Please remove the assertEq and fill, so it works on jsc and v8

Comment 36

[Nicolas B. Pierron [:nbp]]

Attachment: Add forEach micro benchmarks to the assorted test suite.

Delta:
 - Add s JS version of Array.prototype.fill.
 - Add a JS version of assertEq.
 - Use a variable for max_n.

Comment 37

[Hannes Verschore [:h4writer]]

Comment on attachment 8523915 [details] [review]
Add forEach micro benchmarks to the assorted test suite.

This has landed in the beginning of the week IIRC

Comment 38

[Nicolas B. Pierron [:nbp]]

(old try) https://treeherder.mozilla.org/#/jobs?repo=try&revision=194f4ba2be53
(new try) https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21c3ad179f32
(inbound)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c687eceec200
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adb9a599879
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7733d2b6271
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2564ebc7733
https://hg.mozilla.org/integration/mozilla-inbound/rev/f262ebee6242
https://hg.mozilla.org/integration/mozilla-inbound/rev/2450ea9f9c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/a50d660f09da
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c56320cd175

Comment 39

[Nicolas B. Pierron [:nbp]]

(part 2.5 - Fix rooting analysis in computeScopeChain. r=jandem, reviewed over IRC)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1950dd48e705

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c687eceec200
https://hg.mozilla.org/mozilla-central/rev/0adb9a599879
https://hg.mozilla.org/mozilla-central/rev/c7733d2b6271
https://hg.mozilla.org/mozilla-central/rev/c2564ebc7733
https://hg.mozilla.org/mozilla-central/rev/f262ebee6242
https://hg.mozilla.org/mozilla-central/rev/2450ea9f9c62
https://hg.mozilla.org/mozilla-central/rev/a50d660f09da
https://hg.mozilla.org/mozilla-central/rev/0c56320cd175
https://hg.mozilla.org/mozilla-central/rev/1950dd48e705