simplify the lazy-arguments optimization

RESOLVED DUPLICATE of bug 733950

Status

()

defect
RESOLVED DUPLICATE of bug 733950
8 years ago
8 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #588583 - Flags: review?(bhackett1024)
Posted patch simplify lazy args (obsolete) — Splinter Review
This patch does the deed by adding a static method to fall back upon when doing getelem for an out-of-range index.
Attachment #588584 - Flags: review?(bhackett1024)
Oops, forgot to 'put' any existing args objects when setting script->needsArguments to false.  With that, should be green on try.
Attachment #588584 - Attachment is obsolete: true
Attachment #588584 - Flags: review?(bhackett1024)
Attachment #588940 - Flags: review?(bhackett1024)
Attachment #588583 - Flags: review?(bhackett1024) → review+
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.
Attachment #588940 - Flags: review?(bhackett1024) → review+
(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.
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.)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 733950
You need to log in before you can comment on or make changes to this bug.