718134 - simplify the lazy-arguments optimization

Status: RESOLVED DUPLICATE of bug 733950

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

The attached patch uses the newer/better engine invariants provided by bug 718022 to simplify how we deal with lazy arguments.  Note: this patch does not actually eagerly create arguments objects and thus does not have to deal with not losing the 'f.apply(arguments)' optimization.  That is for a later patch.

Surprisingly, this patch is a ~100 point speedup on v8-raytrace and a 8% speedup on:

  function f(x) { return x }
  function g() { return f.apply(null, arguments) }
  var s = 0;
  for (var i = 0; i < 10000000; ++i)
      s += g();

Since the patch doesn't touch hot paths, I think the speedup is from jit-compiling with !script->needsArguments (which allows better optimization).  Taking out the "s +=" makes before/after go the same speed.

Comment 1

[Luke Wagner [:luke]]

Attachment: preparatory patch

This patch un-unions args.nactual and args.obj since, with the next patch, it is possible to have an args.obj when jit code expects args.nactual.  It also does a few minor cleanups.

Comment 2

[Luke Wagner [:luke]]

Attachment: simplify lazy args

This patch does the deed by adding a static method to fall back upon when doing getelem for an out-of-range index.

Comment 3

[Luke Wagner [:luke]]

Attachment: simplify lazy args (v.2)

Oops, forgot to 'put' any existing args objects when setting script->needsArguments to false.  With that, should be green on try.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 588940 [details] [diff] [review]
simplify lazy args (v.2)

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

::: js/src/jsscript.h
@@ +465,5 @@
>      bool            strictModeCode:1; /* code is in strict mode */
>      bool            compileAndGo:1;   /* script was compiled with TCF_COMPILE_N_GO */
>      bool            usesEval:1;       /* script uses eval() */
> +    bool            needsArguments:1;  /* the script may require an arguments object
> +                                        * NB: this flag is mutable */

This mutable flag creeps me out, with the risk of incorrect or non-optimal behavior should needsArguments be accessed before the lazy args analysis has happened.  The comment doesn't say what needs to be done to correctly use this field.

How about keeping an immutable script->usesArguments (as before), and adding a script->analysis()->needsArgumentsObject() which can ensure/assert that the appropriate analysis has happened.

Comment 5

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #4)
Yeah, it is creepy.  script->analysis()->needsArgumentsObject() sounds attractive.  For the current patch, that would have the nice property that we wouldn't need to do the AllFramesIter fixup (since effectively we'd just be running inference earlier).  In subsequent patches (when creating arguments eagerly and speculating about f.apply(arguments)), though, we would have to deal with script->analysis()->needsArgumentsObject() going from true to false.

Comment 6

[Luke Wagner [:luke]]

I landed the preparatory patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe6463d4547

I will not land the second patch until bug 718022 gets into beta, so this bug will remain open for a while.  (I'll do what comment 4 suggested in the meantime.)

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7fe6463d4547