Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: Make non-definite property accesses faster

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Our property accesses are slower than V8 and JSC when we are not accessing definite properties. E.g. on the micro-benchmark below we have:

d8   :  321 ms
JSC  :  345 ms
JM+TI:  550 ms
Ion  :  986 ms

Now if we remove the "if(a)" from the constructor, the definite property analysis works and we're very fast (no difference with V8/JSC):

Ion   : 203 ms
JM+TI : 231 ms

One thing we can do to optimize non-definite properties is to use information from JM's ICs. If an IC always sees the same shape, Ion can inline the property read directly with a separate shape guard. An advantage of this approach over idempotent IC's is that we can hoist/GVN shape checks separately.

function O(a) {
    if (a) { // Thwart definite property analysis.
        this.x = 1;
        this.y = 2;
        this.z = 3;
    }
}
function f() {
    var o = new O(true);
    var t = new Date;
    var res = 0;
    for (var i=0; i<100000000; i++) {
        res += o.x + o.y + o.z;
        o.x = o.y = o.z = 3;
    }
    print(new Date - t);
    print(res);
}
f();
(Assignee)

Updated

5 years ago
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Blocks: 771738
(Assignee)

Updated

5 years ago
Depends on: 773586
(Assignee)

Updated

5 years ago
Depends on: 773655
(Assignee)

Comment 1

5 years ago
Created attachment 642580 [details] [diff] [review]
Part 1: JM

I tried to read the shape out of the inline path, but it turned out to be pretty complicated, especially on ARM where multiple instructions are used to load the immediate, so this patch stores the Shape in the PICInfo.

Eventually we will probably want to store a pointer to another data structure so that we can also store info about the generated stubs etc.
Attachment #642580 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

5 years ago
Created attachment 642600 [details] [diff] [review]
Part 2: Ion

Inline monomorphic get/set in Ion.
Attachment #642600 - Flags: review?(dvander)
(Assignee)

Comment 3

5 years ago
Created attachment 642641 [details] [diff] [review]
Part 3: Invalidate if GuardShape fails

If GuardShape fails, we see a new shape for the first time, so we want to invalidate the current IonScript and run again in JM so that we can collect information about observed shapes.
Attachment #642641 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

5 years ago
Created attachment 642643 [details] [diff] [review]
Part 4: Make JM/Ion heuristics a bit more robust

This ensures we actually run most of v8-richards in Ion. I want to look into this a bit more later, but in the meantime this is a clear v8 win.
Attachment #642643 - Flags: review?(dvander)
Attachment #642580 - Flags: review?(bhackett1024) → review+
Attachment #642600 - Flags: review?(dvander) → review+
Attachment #642643 - Flags: review?(dvander) → review+
Comment on attachment 642641 [details] [diff] [review]
Part 3: Invalidate if GuardShape fails

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

Fix the JSScript retrieval and ask again for review ;)

::: js/src/ion/Bailouts.cpp
@@ +522,5 @@
>  uint32
>  ion::BoundsCheckFailure()
>  {
>      JSContext *cx = GetIonContext()->cx;
> +    JSScript *script = GetTopIonJSScript(cx);

Bad news: Both are wrong.

cx->fp()->script()  will return the frame & script of the innermost inlined script and not the script which is holding the Ion script.

GetTopIonJSScript()  returns the script of the previous frame which called the bailed frame.

The good news is that you might be able to write a function named GetBailedJSScript which extract the calleeToken from the exit frame.  This is safe since the exit frame was a JS frame which hit a bailout.  As we are not relocating the frame and thus erasing the calleeToken, you might cast the exit frame to a JS frame and use the same trick as GetTopIonJSScript to get the JSScript.

@@ +546,5 @@
> +
> +    JS_ASSERT(script->hasIonScript());
> +    JS_ASSERT(!script->ion->invalidated());
> +
> +    IonSpew(IonSpew_Invalidate, "Forced invalidation bailout");

I would prefer if we use a more descriptive spew here.

     Invalidating due to guard shape failure

and replace Bailout_Invalidate by Bailout_GuardShape.
Attachment #642641 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 6

5 years ago
Created attachment 643454 [details] [diff] [review]
Part 3: Invalidate if GuardShape fails (v2)

Now with the GetBailedJSScript you added (thanks). I don't think we should rename Bailout_Invalidate to Bailout_GuardShape though, we want to reuse Bailout_Invalidate for other instructions and can get the name of the LIR instruction from the bailout spew.
Attachment #642641 - Attachment is obsolete: true
Attachment #643454 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

5 years ago
Attachment #643454 - Attachment is patch: true
Comment on attachment 643454 [details] [diff] [review]
Part 3: Invalidate if GuardShape fails (v2)

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

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +232,5 @@
> +    masm.j(Assembler::LessThan, &boundscheck);
> +
> +    // Force invalidation.
> +    {
> +        masm.setupUnalignedABICall(0, edx);

nit: edx -> rdx.
Attachment #643454 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/4d458fb004b6
https://hg.mozilla.org/projects/ionmonkey/rev/4d18d3c3f50f
https://hg.mozilla.org/projects/ionmonkey/rev/eb6bc31ebf63
https://hg.mozilla.org/projects/ionmonkey/rev/c04e9a32ff53

Looks like this was a richards and deltablue win, and the heuristics change made Ion+JM about as fast as Ion on v8-earley.

v8-raytrace regressed though, so the total score did not improve much. Pretty sure that's just because we use Ion more now and we suck on raytrace. I will look into v8-raytrace now.

SS also regressed slightly, going to investigate that too.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 781364
You need to log in before you can comment on or make changes to this bug.