ES5 getters don't trace due to profiler selecting methodjit

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: billm)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, )

Attachments

(1 attachment)

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; });
Reporter

Updated

8 years ago
Blocks: 626021
No longer depends on: 559813, 626021
Reporter

Updated

8 years ago
Summary: ES5 getters/setters are ridiculously slow → ES5 getters don't trace due to profiler selecting methodjit
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
It's an easy fix. I'll post a patch soon.
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.
Reporter

Comment 4

8 years ago
(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.
Yeah, definitely, we should IC ES5 getters/setters and inline them once the overall machinery to do is in place.
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
Posted patch patchSplinter Review
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?
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #517815 - Flags: review?(dmandelin)
Attachment #517815 - Flags: review?(dmandelin) → review+
http://hg.mozilla.org/mozilla-central/rev/f819bfaf8d20
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.