915763 - Remove TypeScript::dynamicList, MonitorOverflow and MonitorString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We no longer need to keep track of this I think. Ion now uses Baseline IC's to determine whether an operation overflowed.

Removing them should speed up the interpreter and JIT VM calls a bit and should be a small memory win.

The only problem I can think of is that we probably still want to trigger recompilation when an operation overflows. Maybe the Baseline IC's could do this when we attach a new stub or only when we attach a new stub for arithmetic operations.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch turned out bigger than I expected, though most of it is code removal:

 25 files changed, 133 insertions(+), 480 deletions(-)

It does the following:

* Removes TypeScript::dynamicList, MonitorOverflow, MonitorString etc.

* The ITERNEXT unboxing in IonBuilder is now fallible and based on a Baseline IC hint. If we ever want to optimize this, I think the right thing to do is track the type of iterator object.

* Removes some BailoutKinds we no longer need.

* Adds a new BailoutKind that's used for the ITERNEXT unbox and when arithmetic operations overflow. This ensures the script is invalidated when this happens.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 810742 [details] [diff] [review]
Patch

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

::: js/src/vm/Interpreter.cpp
@@ -186,5 @@
>      RootedValue value(cx);
>      if (!JSObject::getProperty(cx, obj, obj, cx->names().noSuchMethod, &value))
>          return false;
>  
> -    TypeScript::MonitorUnknown(cx);

Forgot about this one. It shouldn't be removed I think, I can add MonitorUnknown back and just make it add UnknownType() to the op's observed set...

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Forgot about this one. It shouldn't be removed I think, I can add
> MonitorUnknown back and just make it add UnknownType() to the op's observed
> set...

On second thought, when we call OnUnknownMethod we call TypeScript::Monitor anyway. Can you confirm it's okay to remove the MonitorUnknown call from OnUnknownMethod?

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > Forgot about this one. It shouldn't be removed I think, I can add
> > MonitorUnknown back and just make it add UnknownType() to the op's observed
> > set...
> 
> On second thought, when we call OnUnknownMethod we call TypeScript::Monitor
> anyway. Can you confirm it's okay to remove the MonitorUnknown call from
> OnUnknownMethod?

I think that since we're in the VM TI doesn't have anything to say about what type will be produced by the read (e.g. it doesn't capture values for missing properties), so there should be some later Monitor call or jitcode type barrier which executes and captures the value.

Comment 5

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f98f80d2126c

Try: https://tbpl.mozilla.org/?tree=Try&rev=30f9d77eae7a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f98f80d2126c

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 810742 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +8520,5 @@
>          // instructions.
>          spew("Inlining monomorphic GETPROP");
>  
>          Shape *objShape = shapes[0];
> +        obj = addShapeGuard(obj, objShape, Bailout_ShapeGuard);

Why can we change this to Bailout_ShapeGuard. I would think this should get altered to Bailout_BaselineInfo, since this is based on info on baseline?

@@ +8925,5 @@
>          // 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.
>          Shape *objShape = shapes[0];
> +        obj = addShapeGuard(obj, objShape, Bailout_ShapeGuard);

Here the same...

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 810742 [details] [diff] [review]
Patch

(In reply to Hannes Verschore [:h4writer] (PTO:Sept 23-27) from comment #7)
> Why can we change this to Bailout_ShapeGuard. I would think this should get
> altered to Bailout_BaselineInfo, since this is based on info on baseline?

I noticed that Bailout_CachedShapeGuard did exactly the same as Bailout_ShapeGuard so I removed Bailout_CachedShapeGuard and used Bailout_ShapeGuard instead.

We could use Bailout_BaselineInfo as well but Bailout_ShapeGuard sets script->failedShapeGuard to avoid frequent recompilation due to shape guards.