Closed Bug 806793 Opened 7 years ago Closed 7 years ago

IonMonkey: set ShapeGuard not movable after invalidation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: h4writer, Assigned: h4writer)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Reference: crypto.js:243

TI reports when one or multiple shapes goes through a load property/store property. Therefore we use different MIR's. When only one shape is observable we use MShapeGuard and LoadPropSlot etc.  MShapeGuard can be hoisted. This can be a problem. E.g. when multiple shapes go through a function, but one branch only see one shape. Then we will optimize that one branch with GuardShape. During LICM we will hoist it in the path where multiple shapes are visible and therefore we will invalidate the whole time.

Plan of attack:
Just like boundscheck set ShapeGuard not movable after invalidation because of ShapeGuard.
Assignee: general → hv1989
Blocks: 768572
I've renamed Bailout_Invalidate to Bailout_InvalidateShapeGuard, because it was only used for this. Also I didn't want anybody mistakenly use this bailout type, because it will also disable hoisting shapeGaurds when encountered.
Attachment #676577 - Flags: review?(jdemooij)
Crypto probably won't move or just slightly with this patch. This problem is disguised by the recompilations of 431. I'm now busy with that one.
Could we avoid invalidation entirely by not hoisting ShapeGuard if there are multiple ShapeGuards on the same MIR? (I don't know if that applies here.)
(In reply to David Anderson [:dvander] from comment #3)
> Could we avoid invalidation entirely by not hoisting ShapeGuard if there are
> multiple ShapeGuards on the same MIR? (I don't know if that applies here.)

If I understand your statement correctly, the answer is no. It is not related to the number of ShapeGuards on the same MIR.

Example testcase to show the idea.
The property get/set of t.s in function test will only happen on Type1. Therefore a GuardShape will be used.
Notice that both type Type1 and Type2 flow through the function.
So as soon as we hoist the Guardshape above the "if", we will bail every second time.

function test(t, extra) {
   if (extra == 1) {
      t.s = 2;
   } 
}

var t1 = new Type1();
t1.s = 0;

var t2 = new Type2();

for (var i=0; i < 100000; i++) {
  test(t1, 1);
  test(t2, 2);
}
Ah, that is unfortunate. I believe V8 doesn't hoist shape guards from |if| blocks at all, for this very reason.
Comment on attachment 676577 [details] [diff] [review]
disable hoisting guardShape after invalidating because of guardShape

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

Nice work, r=me with comments addressed.

::: js/src/ion/Bailouts.cpp
@@ +313,5 @@
>          return BAILOUT_RETURN_RECOMPILE_CHECK;
>        case Bailout_BoundsCheck:
>          return BAILOUT_RETURN_BOUNDS_CHECK;
> +      case Bailout_InvalidateShapeGuard:
> +        return BAILOUT_RETURN_INVALIDATE_SHAPE_GUARD;

Nit: can you rename this to Bailout_ShapeGuard and BAILOUT_RETURN_SHAPE_GUARD?

@@ +551,5 @@
>      return true;
>  }
>  
>  uint32
> +ion::InvalidateShapeGuardFailure()

Nit: ShapeGuardFailure

@@ +560,5 @@
>      JS_ASSERT(script->hasIonScript());
>      JS_ASSERT(!script->ion->invalidated());
>  
> +    if (!script->failedShapeGuard)
> +        script->failedShapeGuard = true;

Nit: no need for the if(..), just always assign "true".

@@ +577,5 @@
>      JS_ASSERT(script->hasIonScript());
>      JS_ASSERT(!script->ion->invalidated());
>  
> +    if (!script->failedShapeGuard)
> +        script->failedShapeGuard = true;

Same here.

::: js/src/ion/IonBuilder.cpp
@@ +6109,5 @@
>              // The JM IC was monomorphic, so we inline the property access as
>              // long as the shape is not in dictionary mode. We cannot be sure
>              // that the shape is still a lastProperty, and calling Shape::search
>              // on dictionary mode shapes that aren't lastProperty is invalid.
> +            addShapeGuard(obj, objShape, Bailout_CachedShapeGuard);

obj = addShapeGuard(...)

@@ +6439,5 @@
>      return check;
>  }
> +
> +MInstruction *
> +IonBuilder::addShapeGuard(MDefinition *obj, const Shape *shape, BailoutKind bailoutKind) 

Nit: trailing whitespace.
Attachment #676577 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/505785fec80e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.