8x slower on dart2js output of Tracer benchmark vs v8

RESOLVED FIXED in mozilla39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: azakai, Assigned: jandem)

Tracking

({perf})

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Posted file 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.
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.
Flags: needinfo?(jdemooij)
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);
Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8514209 - Flags: review?(bhackett1024)
Attachment #8514209 - Flags: review?(bhackett1024) → review+
Flags: needinfo?(jdemooij)
Keywords: leave-open
Flags: needinfo?(jdemooij)
Forgot the link, sorry for the bugspam. Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eeb6a55facb
Depends on: 1129387
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.
(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.
Depends on: 1106828
Keywords: perf
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.
Flags: needinfo?(jdemooij)
Attachment #8571892 - Flags: review?(bhackett1024)
Attachment #8571892 - Flags: review?(bhackett1024) → review+
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.
Attachment #8572818 - Flags: review?(bhackett1024)
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.
Attachment #8572818 - Flags: review?(bhackett1024) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.