Open Bug 936156 Opened 6 years ago Updated 6 years ago

TI new-script analysis doesn't work well with TypeScript-generated JS

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

People

(Reporter: jandem, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Whiteboard: [leave open])

Attachments

(2 files)

As discussed on IRC, for some reason we don't run the definite properties analysis on the ASTSpan function in Octane-Typescript. It looks like the same TypeObject is used for multiple constructors. Since this is generated code, the same pattern will probably also show up elsewhere.

A related issue is the AST constructor: it would be great if we could inline _super.call(this) when running the definite properties analysis.
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
There are a couple issues going on here.  First, on nested run once lambdas like:

(function () {
    ....
    (function () { ... })();
})()

We don't mark the inner run once lambda as such, which keeps inner functions from being given singleton types and inhibits other optimizations.  Nested run once lambdas seem to be used extensively in TypeScript.

Second, we never ever inline foo.call(what), due to what looks like a simple bug --- I think it's trying to avoid inlining foo.call().

With this patch, ASTSpan and other functions are inlined fine during the definite properties analysis, and we seem to get the appropriate number of properties added, e.g. ASTList (which inherits from ASTSpan) picks up 12 definite properties.

This gives a 14% boost to the TypeScript score for me, testing on x64 (8633 -> 9857).

ASTSpan doesn't get analyzed itself by the definite properties analysis because of the __extends function below.

var __extends = this.__extends || function (d, b) {
    function __() { this.constructor = d; }
    __.prototype = b.prototype;
    d.prototype = new __();
};

The 'new __()' is constructing a new object with the same prototype as ASTSpan, when b == ASTSpan.  The object 'new type' stuff can't cope with the same prototype being used for constructing different functions (or non-functions) so the 'new type' for the prototype is not specialized for any one function.  This could be fixed pretty easily by reworking the 'new type' stuff to include the function in the hashtable key (the current mechanism was carried over from the junky old days when the 'new type' was directly pointed to by the prototype object).
Assignee: nobody → bhackett1024
Attachment #8342641 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Comment on attachment 8342641 [details] [diff] [review]
patch

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

Nice finds.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5509,5 @@
>           * accessed via foo.caller indirection), as multiple executions
>           * will just cause the inner scripts to be repeatedly cloned.
>           */
>          JS_ASSERT(!bce->emittingRunOnceLambda);
> +        if (bce->checkSingletonContext() || (!bce->isInLoop() && bce->isRunOnceLambda())) {

Just curious, can't we add this to checkSingletonContext so that it returns |true| here?
Attachment #8342641 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +5509,5 @@
> >           * accessed via foo.caller indirection), as multiple executions
> >           * will just cause the inner scripts to be repeatedly cloned.
> >           */
> >          JS_ASSERT(!bce->emittingRunOnceLambda);
> > +        if (bce->checkSingletonContext() || (!bce->isInLoop() && bce->isRunOnceLambda())) {
> 
> Just curious, can't we add this to checkSingletonContext so that it returns
> |true| here?

checkSingletonContext() returns true if we know the code will run only once, while we can't guarantee that run once lambdas will actually only execute once (arguments related trickery can be used to recover the lambda and call it again).  We use checkSingletonContext when deciding to emit JSOP_OBJECT, and if the same JSOP_OBJECT runs multiple times it will return the same physical object.  This could be worked around by deep cloning in JSOP_OBJECT (bug 920322) though that incurs additional cpu/memory cost.  The other place we use isRunOnceLambda for is assigning functions singleton types, and that has mechanisms in place to cope with run once lambdas running multiple times.
This patch incorporates the new function into the new type objects hashtable, so that using the same prototype when constructing different functions will use different type objects.
Attachment #8344968 - Flags: review?(jdemooij)
Comment on attachment 8344968 [details] [diff] [review]
incorporate new function into new type objects

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

::: js/src/jsinfer.cpp
@@ -3730,5 @@
> - * This class is used to add a post barrier on the newTypeObjects set, as the
> - * key is calculated from a prototype object which may be moved by generational
> - * GC.
> - */
> -class NewTypeObjectsSetRef : public BufferableRef

This class is still used in ExclusiveContext::getNewType #ifdef JSGC_GENERATIONAL.
Attachment #8344968 - Flags: review?(jdemooij) → review+
Duplicate of this bug: 945287
Depends on: 1008339
You need to log in before you can comment on or make changes to this bug.