Closed
Bug 771130
Opened 12 years ago
Closed 12 years ago
IonMonkey: Make non-definite property accesses faster
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Inline monomorphic get/set in Ion.
Attachment #642600 -
Flags: review?(dvander)
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #642580 -
Flags: review?(bhackett1024) → review+
Updated•12 years ago
|
Attachment #642600 -
Flags: review?(dvander) → review+
Updated•12 years ago
|
Attachment #642643 -
Flags: review?(dvander) → review+
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #643454 -
Attachment is patch: true
Comment 7•12 years ago
|
||
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•12 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
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•