Last Comment Bug 621937 - JM: TypeInference: re-enable assignment ICs on monitored assignments
: JM: TypeInference: re-enable assignment ICs on monitored assignments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: infer-perf-regress 639508 642412 LandTI
  Show dependency treegraph
 
Reported: 2010-12-29 10:51 PST by Brian Hackett (:bhackett)
Modified: 2011-06-07 19:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Brian Hackett (:bhackett) 2010-12-29 10:51:31 PST
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 (:bhackett) 2011-03-11 10:29:08 PST
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 (:bhackett) 2011-03-11 16:37:28 PST
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 (:bhackett) 2011-03-11 16:38:10 PST
Er, those ratios are for the V8 benchmark.
Comment 4 Brendan Eich [:brendan] 2011-03-12 13:28:03 PST
(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 (:bhackett) 2011-03-12 23:12:45 PST
> 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] 2011-03-14 00:46:35 PDT
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] 2011-06-07 18:30:08 PDT
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 (:bhackett) 2011-06-07 19:09:45 PDT
This was fixed a while back, forgot to resolve this bug.

*** This bug has been marked as a duplicate of bug 619271 ***
Comment 9 Brian Hackett (:bhackett) 2011-06-07 19:12:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.