Closed Bug 843937 Opened 9 years ago Closed 9 years ago

IonMonkey: specialize on eval("name()")

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bhackett1024, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
As outlined in bug 842522, the eval's in ss-date-format-tofte are of the form eval("name()").  It would be nice to specialize on this and skip the construction and entering of the eval script, which will just call back into a regular function.  The attached patch does this for IonMonkey.  I get these times on a date-format-tofte modified to run 20x longer:

d8: 167
trunk, pre-743394: 450
trunk, post-743394: 385
trunk, with patch: 310

So, this plus 743394 cut out about half of the difference between us and v8.  I believe the remainder is primarily (a) a lack of inlined natives for Date functions like getHours(), and (b) a lack of ggc (tofte's main function creates a lot of garbage arrays and lambdas).

Of course, date-format-tofte runs short enough that the benefits of Ion optimizations will not be fully realized.  On trunk the main function in tofte is not compiled by Ion until practically the end of the benchmark, so there won't be any benefit to doing this.  With the baseline compiler, we compile much sooner so the main function should (need to double check) run about 90% of the time as Ion code.

This patch is pretty woefully special cased to the format of the eval calls in tofte.  It would be nice to beef this up at some point and deal with other simple eval forms (eval("name"), eval("obj.prop"), ...), but doing so would require feedback from baseline about what sort of strings have flowed through the eval previously.

Also, please pay attention to the use of resume points, safepoints and snapshots around the VM call made to do a dynamic name lookup.  This call can't fail or invalidate, but still seems to require the associated machinery (is there a way to avoid this?) and has a bailout afterwards.
Attachment #716914 - Flags: review?(jdemooij)
(In reply to Brian Hackett (:bhackett) from comment #0)
> Also, please pay attention to the use of resume points, safepoints and
> snapshots around the VM call made to do a dynamic name lookup.  This call
> can't fail or invalidate, but still seems to require the associated
> machinery (is there a way to avoid this?) and has a bailout afterwards.

You can use masm.callWithABI instead of callVM if the VM call cannot GC. It's a bit faster and doesn't require a safepoint, resumepoint etc. See for instance LModD codegen.
Attached patch updatedSplinter Review
Updated to use callWithABI instead of callVM.  This improves the time from 310 to 300.
Attachment #716914 - Attachment is obsolete: true
Attachment #716914 - Flags: review?(jdemooij)
Attachment #717094 - Flags: review?(jdemooij)
Comment on attachment 717094 [details] [diff] [review]
updated

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

Nice.

::: js/src/ion/CodeGenerator.cpp
@@ +1684,5 @@
> +    masm.adjustStack(-int32_t(sizeof(Value)));
> +    masm.movePtr(StackPointer, temp2);
> +
> +    masm.setupUnalignedABICall(4, temp1);
> +    masm.passABIArg(temp1);

This should be temp3.
Attachment #717094 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/6f8c5793be7d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.