Last Comment Bug 771130 - IonMonkey: Make non-definite property accesses faster
: IonMonkey: Make non-definite property accesses faster
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: general
Mentors:
Depends on: 773586 773655 781364
Blocks: IonSpeed 771738
  Show dependency treegraph
 
Reported: 2012-07-05 06:58 PDT by Jan de Mooij [:jandem]
Modified: 2012-08-08 15:26 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: JM (5.98 KB, patch)
2012-07-16 07:39 PDT, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Review
Part 2: Ion (16.36 KB, patch)
2012-07-16 08:37 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review
Part 3: Invalidate if GuardShape fails (15.39 KB, patch)
2012-07-16 10:43 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Review
Part 4: Make JM/Ion heuristics a bit more robust (3.32 KB, patch)
2012-07-16 10:47 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review
Part 3: Invalidate if GuardShape fails (v2) (15.20 KB, patch)
2012-07-18 11:00 PDT, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Review

Description Jan de Mooij [:jandem] 2012-07-05 06:58:42 PDT
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();
Comment 1 Jan de Mooij [:jandem] 2012-07-16 07:39:45 PDT
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.
Comment 2 Jan de Mooij [:jandem] 2012-07-16 08:37:55 PDT
Created attachment 642600 [details] [diff] [review]
Part 2: Ion

Inline monomorphic get/set in Ion.
Comment 3 Jan de Mooij [:jandem] 2012-07-16 10:43:56 PDT
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.
Comment 4 Jan de Mooij [:jandem] 2012-07-16 10:47:00 PDT
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.
Comment 5 Nicolas B. Pierron [:nbp] 2012-07-16 11:49:54 PDT
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.
Comment 6 Jan de Mooij [:jandem] 2012-07-18 11:00:49 PDT
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.
Comment 7 Nicolas B. Pierron [:nbp] 2012-07-18 15:48:53 PDT
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.
Comment 8 Jan de Mooij [:jandem] 2012-07-19 04:37:37 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.