638794 - TI: JM: Specialize on Prototype library's create() methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Objects constructed with 'new' using the Prototype library (and v8-raytrace benchmark) all get funneled through a common script, which looks like:

function klass() {
    this.initialize.apply(this, arguments);
}

There are many instances of this function, each with a different .prototype and a different .initialize.  This polymorphism tends to destroy type information, but inference could retain it here with JIT entry points in each of the initialize functions that type check their arguments (probably just add these to the arity check entry point rather than making a new one; separate bug).

We could do even better (faster) though, by specializing on this code pattern and inlining its behavior both within inference and within JM itself.  This could be done without a generalized scripted call inlining mechanism, and would be faster besides by optimizing out the apply.

After talking with Brendan on IRC, there are no problems with erasing stack frames by inlining.  The main challenge is making sure recompilation works right if .initialize or something it calls scribbles all over the .initialize or .apply property.  This shouldn't be hard, as the 'apply' is a tail call.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: WIP

WIP improving analysis precision and performance around Prototype.js uses of Object.extend and Class.create.  The former marks singleton types for 'new' and {} objects created in global scope, which we can track property types precisely for when they have had arbitrary properties added with string SETELEM.  The latter does some pattern matching and robustness improvements to JM so that constructors and mismatched arg calls can be inlined, with special handling for doing apply(arguments) within an inlined frame (where we know exactly what the arguments are).

Color = Class.create();

Color.prototype = {
    red : 0.0,
    green : 0.0,
    blue : 0.0,

    initialize : function(r, g, b) {
        this.red = r;
        this.green = g;
        this.blue = b;
    },

    sum: function() { return this.red + this.green + this.blue; }
}

function bar() {
  for (var i = 0; i < 100; i++) {
    var Klass = Class.create();
    Klass.prototype = { initialize: function() {} }
    for (var j = 0; j < 10; j++)
      new Klass();
  }
}
bar();

function foo() {
  for (var i = 0; i < 100000; i++) {
    var a = new Color(1,2,3);
    a.sum();
  }
}
for (var i = 0; i < 40; i++) {
  foo();
  if (typeof gc != "undefined")
    gc();
}

js -m -n:  318
js -m:     555
js -j:     1460
d8:        376

After removing the 'bar()' call, making accesses in Class.create monomorphic:

js -m -n:  285
js -m:     551
js -j:     1453
d8:        205

Comparable code written in a normal JS style --- function Color() {}, Color.prototype.sum = ...

js -m -n:  288
js -m:     355
js -j:     366
d8:        57

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Patch effective on v8-raytrace.  Improves our raytrace score by about 40% (x86 shell, 3774 -mjp -> 5280 -mn).  Using -D still points to a fair amount of overhead while constructing Color and Vector objects (the main objects constructed by this benchmark).

Color.initialize:

    initialize : function(r, g, b) {
        if(!r) r = 0.0;
        if(!g) g = 0.0;
        if(!b) b = 0.0;

        this.red = r;
        this.green = g;
        this.blue = b;
    },

Vector.initialize:

    initialize : function(x, y, z) {
        this.x = (x ? x : 0);
        this.y = (y ? y : 0);
        this.z = (z ? z : 0);
    },

r/g/b and x/y/z are never undefined (no argc < nargs) and known to be doubles, so in principle all these 'if' statements could be optimized away with the type information we have.  Removing these manually gains 800 points on the benchmark.  This may be worth doing for IonMonkey, but this sort of peephole optimizing is hard to do in JM2.  Though we do take a stub for the (!r) etc. which should get fixed.

We also don't compute definite slots for red/green/blue and x/y/z, as the NewScript stuff can't see through the Class.create which 'new' was invoked on.  The hottest property access sites in the benchmark are on these properties.  It shouldn't be hard to fix this, but would rather do this as a followup rather than let this patch metastasize further.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

In case it's helpful, this patch applies cleanly to 78a56e48dd3c.

Comment 4

[Paul Wright]

Is there a plan to complete this bug other than the portion that would be needed to fix Bug 731398?

Comment 5

[Brian Hackett [Laid off!]]

Yes, but for IM rather than JM.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: distinguish different create methods

Split out and cleanup some of the above (badly metastasized) patch, to improve type information around calls to methods wrapped via Prototype.js Class.create().  This works by trying to recognize inner functions which are generic wrappers --- including both .apply() and arguments --- and clones the inner function and entire associated script whenever new instances of the wrapper are created.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 642976 [details] [diff] [review]
distinguish different create methods

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

Thanks for fixing this.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/120e3d9d86f1

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/120e3d9d86f1