JM: TypeInference: re-enable assignment ICs on monitored assignments

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
Type inference currently disables ICs for writes of properties via setprop/setname/setelem, when inference cannot determine the type of the object being modified.  This needs to get fixed, and should not be too hard now that bug 619271 is fixed.

Inference needs to overapproximate the types of each property of all objects; with 619271, the shape guard in the IC determines the type of the modified object, so during generation of the IC stub we can account for all possible updates on that property of objects of that type, by marking the property's type as unknown.
(Reporter)

Updated

6 years ago
Blocks: 639508
(Reporter)

Comment 1

6 years ago
The only remaining issue now is that Call objects from different scripts can share the same shape.  For SETNAME ICs to be able to capture their type effects completely, the shape guard needs to determine the correct type set to update (i.e. the per-script local or arg being written).

The simple solution is to stop this sharing, i.e. remove compartment->emptyCallShape and use a new EmptyShape (and subsequent shape tree/linear chain) for each JSScript.

Another option would be to remember the static nesting of scripts and look for sharing when generating the Call object IC, but this runs into problems with JS_CloneFunctionObject.

Any comments or other ideas (I don't know the Bindings code well) before I go ahead with the first approach?
(Reporter)

Comment 2

6 years ago
Patch to reenable PICs on SETNAME and SETPROP. This does not handle SETELEM yet, which is more involved --- the important case there is for dense arrays, which are non-native so are guarded with clasp (which does not determine type) rather than shape. Will probably need to slow this path down and switch on the object's type for these. Also, this improves TI+JM from being 3.1 times slower than stock JM to being 1.8 times slower (most of the remainder is bug 621942 and bug 619428).

http://hg.mozilla.org/projects/jaegermonkey/rev/bcf148dbce2f
(Reporter)

Comment 3

6 years ago
Er, those ratios are for the V8 benchmark.
(In reply to comment #1)
> The only remaining issue now is that Call objects from different scripts can
> share the same shape.  For SETNAME ICs to be able to capture their type effects
> completely, the shape guard needs to determine the correct type set to update
> (i.e. the per-script local or arg being written).
> 
> The simple solution is to stop this sharing, i.e. remove
> compartment->emptyCallShape and use a new EmptyShape (and subsequent shape
> tree/linear chain) for each JSScript.

This might be ok. What was not ok, which led to the maximal and heretofore safe sharing of emptyCallShape, was a unique shape per Call object. We fell into that default-bad case inadvertently -- see bug 592534.

What is the problem with JS_CloneFunctionObject? Precompilation with cloning for different runtime scope chains should be orthogonal.

/be
(Reporter)

Comment 5

6 years ago
> What is the problem with JS_CloneFunctionObject? Precompilation with cloning
> for different runtime scope chains should be orthogonal.
> 
> /be

The issue is that trying to impose a static type nesting for scripts doesn't make sense when there is a way to reparent functions to arbitrary scripts.  The former approach used in this bug does not have problems with cloning functions and fits the code's design better of letting scripts run with arbitrary parent chains, modulo UPVAR and GNAME accesses.
One thing not obvious from the API: we never reparent to arbitrary scope. The new runtime parent is either a distinct global, or else a Call object for a function object that was cloned from the enclosing compiler-created function.

/be

Updated

6 years ago
Blocks: 642412
Are these disabled in the TI branch, or only if TI is enabled? If the former, this needs to be fixed before landing. If the latter, this can be disconnected from landing.
Blocks: 662702
(Reporter)

Comment 8

6 years ago
This was fixed a while back, forgot to resolve this bug.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 619271
(Reporter)

Comment 9

6 years ago
Sorry, actually this bug is self contained.  It was left open because we still stub SETELEM when the type of the lhs is unknown (since dense array SETELEM does not have a shape guard), but when/if that becomes a problem it can be resolved in a new bug.
Resolution: DUPLICATE → FIXED
You need to log in before you can comment on or make changes to this bug.