1187021 - Force arguments object allocation on dynamic name accesses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

I'm trying to fix bug 889158 and this optimization is getting in the way of that. The details are in that bug, but the most important parts:

(In reply to Jan de Mooij [:jandem] from comment #15)
> I wonder if we should revert bug 842522, "Don't force construction of
> arguments objects on dynamic name accesses". As that bug mentions, at the
> time Ion was unable to compile functions that required non-lazy arguments.
> This is no longer true. Furthermore, with GGC and some other optimizations I
> did last month, creating arguments objects is a lot faster.

I measured this on date-format-tofte:

(In reply to Jan de Mooij [:jandem] from comment #16)
> It seems to be at most a 0.5 ms slowdown there (6.5 -> 7.0 ms) and we can
> remove 200+ lines of code from the frontend and JIT.
> 
> So the question is if there are other cases where it'd hurt to create an
> arguments object when direct eval is used. Considering direct eval already
> kills performance in a lot of ways, and that it's pretty well-known, I think
> it's fair to go ahead with this.

14 files changed, 6 insertions(+), 212 deletions(-)

Comment 1

[Shu-yu Guo [:shu]]

Comment on attachment 8638089 [details] [diff] [review]
Patch

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

Since there are no serious regressions, deleting code is always nice.

::: js/src/jit/VMFunctions.cpp
@@ -600,5 @@
> -    if (!linear)
> -        return false;
> -
> -    static const char16_t arguments[] = {'a', 'r', 'g', 'u', 'm', 'e', 'n', 't', 's'};
> -    static const char16_t eval[] = {'e', 'v', 'a', 'l'};

The only reason we also check 'eval' is because we can't see if the inner eval has 'arguments', right?

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acca05b8182e

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/acca05b8182e

Comment 4

[Hannes Verschore [:h4writer]]

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- ss: format-tofte: 10% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2b568650c03a&tochange=acca05b8182e

More details: http://arewefastyet.com/regressions/#/regression/1795069

Like mentioned in the bug report it was expected to see a format-tofte regression. This is the only regression visible on all slaves/benchmarks we currently run. Marked as wontfix.