Closed Bug 771130 Opened 12 years ago Closed 12 years ago

IonMonkey: Make non-definite property accesses faster

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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: general → jdemooij
Status: NEW → ASSIGNED
Blocks: 771738
Depends on: 773586
Depends on: 773655
Attached patch Part 1: JMSplinter Review
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)
Attached patch Part 2: IonSplinter Review
Inline monomorphic get/set in Ion.
Attachment #642600 - Flags: review?(dvander)
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)
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)
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)
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+
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
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.