639858 - ES5 getters don't trace due to profiler selecting methodjit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The profiler selects the methodjit even on code that uses ES5 getters, a case where the tracer can be much faster. The shell testcase below is fast in the shell with -j, as expected, but with -m or -j -m -p the cases involving a getter are very slow.

I imagine this can be fixed either by making the profiler aware of the difference or by making the methodjit ICs handle getters.

----

function TestClass() {
    this._x = 1;
    this.getX = function() { return this._x; };
    this.__defineGetter__('x1', function() { return this._x; });
    Object.defineProperty(this, "x2", {get: function() { return this._x; }});
}
TestClass.prototype = {get x3() { return this._x; }};
var t = new TestClass();

function test(name, fn) {
    var t0 = new Date;
    fn();
    var dt = new Date - t0;
    print(name + ": " + dt.toFixed(1) + " msec");
}

test('_x', function () { var i$ = 1e6; while (i$--) t._x; });
test('getX()', function () { var i$ = 1e6; while (i$--) t.getX(); });
test('x1', function () { var i$ = 1e6; while (i$--) t.x1; });
test('x2', function () { var i$ = 1e6; while (i$--) t.x2; });
test('x3', function () { var i$ = 1e6; while (i$--) t.x3; });

Comment 1

[Brendan Eich [:brendan]]

Go faster than any non-tracing VM soon via a small profiler patch -- good.

Take longer with a bigger patch, and go only as fast as IC-based competition when they IC ES5 getters -- not as good.

Bill, is this to fix in the profiler as easy as it seems?

/be

Comment 2

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

It's an easy fix. I'll post a patch soon.

Comment 3

[Jeff Walden [:Waldo]]

Is there a reason not to do both (make profiler aware, implement getter/setter ICs) eventually?  Presumably if the profiler, even knowing about this quirk, decides the methodjit is preferred for some code, we'd want the methodjit to not be slow for getters and setters used in it.

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> Is there a reason not to do both (make profiler aware, implement getter/setter
> ICs) eventually?

No, we should. I don't think the methodjit patch would be hugely complicated, either, but I'm not sure.

Let's fix the profiler in this bug. Someone more familiar with the methodjit ICs can file one about making them support accessors.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, definitely, we should IC ES5 getters/setters and inline them once the overall machinery to do is in place.

Comment 6

[Brendan Eich [:brendan]]

Sure, the choices are not exclusive (good, not as good). As usual we end up with "do both: best of all." We are after all paid by the JIT ;-).

/be

Comment 7

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

Attachment: patch

I think this should do it. The main things I'm unsure of are: (1) am I getting the right obj for each variation of JSOP_GETPROP, and (2) is shape->hasGetterValue() the test we want to be making here?

Comment 8

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

http://hg.mozilla.org/tracemonkey/rev/f819bfaf8d20

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/f819bfaf8d20