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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Jan de Mooij [:jandem]]

(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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: updated

Updated to use callWithABI instead of callVM.  This improves the time from 310 to 300.

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8c5793be7d

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6f8c5793be7d