nerf f.arguments so that we can optimize args like locals in JM

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: luke, Assigned: dvander)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
In the general case, because of the arguments object, we have to assume every argument is an upvar in JM.  This means more loads/stores at stub calls and breaks copy propagation.  As an optimization, we might want to mark a script as statically not using 'arguments', however, this isn't good enough because of fun.arguments:

  function f(x) { /* no 'arguments' here */ g(); return x; }
  function g() { f.arguments[0] = "surprise!"; }
  assertEq(f(1), "surprise!")

Now, fun.arguments is unspecified by e262-v3 and a poison-pill on v5, so the only thing binding us here is the wild web.  v8 breaks f.arguments use if it doesn't see 'arguments' statically in a function.  I think JSC gets it right, but I didn't push to hard.  One thing we were talking about that would really help us is to say that any store to an arguments object where there was no static mention of 'arguments' in that function would be a nop.  This would be pretty easy to implement (in ArgSetter + a flag in the script).  Another option is to just return the poison pill for *any* reference to 'arguments' where 'arguments' is not syntactically present in the function.

What do you guys think?
Specifically in v8, if |f| does not statically use the "arguments" keyword, writes to |f.arguments| will not be reflected in |f|'s formal arguments. They don't nop. This is true of JSC as well:

function f(x, y) { x(f); print(y); }
function g(x) { print(x.arguments[1]); x.arguments[1] = "bye"; }
f(g, "hello");

Prints "hello", "hello" in v8/JSC - "hello", "bye" in SpiderMonkey.

function f(x, y) { x(f); print(y); print(arguments[1]); }
function g(x) { print(x.arguments[1]); x.arguments[1] = "bye"; }
f(g, "hello");

Prints "hello", "bye" in all three engines.
(In reply to comment #0)
> In the general case, because of the arguments object, we have to assume every
> argument is an upvar in JM.

It's worse than an upvar as we've used the term. Dynamic scope!

> Now, fun.arguments is unspecified by e262-v3 and a poison-pill on v5,

ES5 strict mode only, not ES5 non-strict.

> What do you guys think?

I think we should match JSC and V8 (comment 1). Sooner is better. Need to test all combinations of interp and JITs.

/be
Someone with Windows should test Chakra (IE9) and report here.

/be
BTW, the closure optimizations (upvar analysis) considers formal parameters to be potentially aliased only if their function's body uses arguments (qualified by dot or not). So we are breaking a la JSC and V8 for upvar-param cases already.

/be
Created attachment 474882 [details] [diff] [review]
fix

This ignores setting aliased arguments in an arguments object if (1) it has an active JSStackFrame, and (2) neither |eval| nor |arguments| were seen at compile time in the script.

This is the minimal change needed to let us optimize arguments in JM, and matches v8 functionality. We could, of course, remove f.arguments harder if desirable.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #474882 - Flags: review?(brendan)
Comment on attachment 474882 [details] [diff] [review]
fix

Nice.

We can get rid of f.arguments next major release, maybe. It might take some time based on ES5 strict and successor (Harmony) versions built on it taking hold with devs. Not now.

/be
Attachment #474882 - Flags: review?(brendan) → review+

Comment 8

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c156bf829377
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.