1090583 - 8x slower on dart2js output of Tracer benchmark vs v8

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Attachment: fromDart.js

Running the dart benchmarks, I noticed that the dart2js version of Tracer (i.e., compiled to JS from dart) runs fairly slowly, around 10x slower than the handwritten Tracer, and in a more apples-to-apples comparison, 8x slower than v8 on the same dart2js output.

Comment 1

[Jan de Mooij [:jandem]]

Ouch, I'm seeing 70% of the time under CreateThisForFunctionWithProtoWrapper, a stub called from Ion JIT code. I'll try to create a smallish testcase.

Comment 2

[Jan de Mooij [:jandem]]

OK, see the micro-benchmark below:

SM: 1334 ms
d8:   72 ms

No wonder we're slower.

The problem is that our constructor calls are awesome and fast if the target function has a singleton type, but if that's not the case we have a slow VM call to construct the `this` object. Bad perf fault that we should fix.

function cons() {
    function Vector(x, y, z) {
	this.x = x;
	this.y = y;
	this.z = z;
    }
    return Vector;
}
var V = cons();
function f() {
    for (var i=0; i<10000000; i++)
	var v = new V(1, 2, 3);
    return v;
}
var t = new Date;
f();
print(new Date - t);

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix getSingletonPrototype to work with non-singletons

Removes 2 lines of code so that getSingletonPrototype works if the target function has non-singleton type.

This fixes the micro-benchmark in comment 2 (from 1300 ms to 70 ms), but the original benchmark needs more work.

Comment 4

[Jan de Mooij [:jandem]]

Forgot the link, sorry for the bugspam. Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eeb6a55facb

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0eeb6a55facb

Comment 6

[Jan de Mooij [:jandem]]

With bug 1129387 fixed we have:

V8:  632 ms
SM: 1161 ms

So we're down from 8x slower to < 2x slower. We spend almost all time in JIT code, so we probably have bad type information somewhere.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> V8:  632 ms
> SM: 1161 ms

The remaining problem is that the definite properties analysis fails for a number of important constructors, because AddClearDefiniteGetterSetterForPrototypeChain returns false because an object on the prototype has unknown properties.

The unknown properties there comes from the MarkObjectGroupUnknownProperties in TypeScript::MonitorAssign: we have a loop copying properties and the heuristic in TypeScript::MonitorAssign kicks in :(

With the MarkObjectGroupUnknownProperties call there commented out I get:

V8: 632 ms
SM: 628-640 ms
SM --unboxed-objects: 563 ms

So, getting rid of that call in TypeScript::MonitorAssign should be a nice win here, especially with --unboxed-objects.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Fix MonitorAssign property count check

This fixes the problem in comment 7, performance improvement is still about the same.

TypeScript::MonitorAssign does:

    if (group->getPropertyCount() < 128)
        return;
    MarkObjectGroupUnknownProperties(cx, group);

The problem is that getPropertyCount overapproximates (returns the capacity) if the properties are stored in a HashSet, so we may hit this when there are only 32 properties. I also think we shouldn't rely on the HashSet implementation for this.

The patch changes it to use basePropertyCount. I kept the 128, it's quite a lot but MarkObjectGroupUnknownProperties is pretty bad for performance so I think it makes sense to do this only if there really are a ton of properties.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Fix pre-existing bug

Part 2 (tweaking the MonitorAssign heuristic) exposed an existing bug when running jit-test/tests/ion/bug904315.js

We were eliminating a LoadElementHole even though it needed a negative int check. This patch sets the guard flag to prevent that.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8572818 [details] [diff] [review]
Fix pre-existing bug

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

::: js/src/jit/MIR.h
@@ +8201,5 @@
>          setMovable();
> +        // Set the guard flag to make sure we bail when we see a negative
> +        // index. We can clear this flag (and needsNegativeIntCheck_) in
> +        // collectRangeInfoPreTrunc.
> +        setGuard();

newlines before/after this comment/call.

Comment 11

[Jan de Mooij [:jandem]]

The bug fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f785209f088d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f785209f088d

Comment 13

[Jan de Mooij [:jandem]]

And part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/594fc93e0ae2

Keeping the leave-open for now because I want to add this test to AWFY.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/594fc93e0ae2

Comment 15

[Hannes Verschore [:h4writer]]

http://arewefastyet.com/#machine=28&view=single&suite=misc&subtest=bugs-1090583-dart2js-tracer