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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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.

Comment 1

[Brian Hackett [Laid off!]]

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?

Comment 2

[Brian Hackett [Laid off!]]

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

Comment 3

[Brian Hackett [Laid off!]]

Er, those ratios are for the V8 benchmark.

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Brian Hackett [Laid off!]]

> 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.

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[David Mandelin [:dmandelin]]

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.

Comment 8

[Brian Hackett [Laid off!]]

This was fixed a while back, forgot to resolve this bug.

Comment 9

[Brian Hackett [Laid off!]]

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.