Closed Bug 915763 Opened 6 years ago Closed 6 years ago

Remove TypeScript::dynamicList, MonitorOverflow and MonitorString

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #810742 - Flags: review?(bhackett1024)
Summary: Remove MonitorOverflow and MonitorString → Remove TypeScript::dynamicList, MonitorOverflow and MonitorString
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...
(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?
(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.
Attachment #810742 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/f98f80d2126c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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...
Attachment #810742 - Flags: feedback?(jdemooij)
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.
Attachment #810742 - Flags: feedback?(jdemooij)
Depends on: 995817
No longer depends on: 995817
You need to log in before you can comment on or make changes to this bug.