Closed Bug 784550 Opened 7 years ago Closed 7 years ago

2x slowdown on emscripten memops benchmark from bug 775323


(Core :: JavaScript Engine, defect)

Not set





(Reporter: azakai, Assigned: luke)



(Whiteboard: [js:p1])


(2 files)

Attached file memops benchmark
The attached benchmark regressed to taking twice as long when bug 775323 landed. (We actually have this on awfy but I guess it wasn't noticed, and it came in through a merge from m-c which perhaps devs look at less.)

Specifically the revision that causes the regression is

changeset:   102511:48cfc16cac71
user:        Luke Wagner <>
date:        Mon Jul 30 11:38:53 2012 -0700
summary:     Bug 775323 - build Bindings after, not during, parsing (r=ejpbruel)
Blocks: 775323
The root problem seems to be a function incorrectly getting marked as having its "bindings accessed dynamically" which causes every access to a local variable to use the slower aliased path.

The cause of this is actually pre-existing: there is a function of the form:
  function a(a) { a = ... }
and the parser incorrectly calls setBindingsAccessedDynamically thinking that the assignment to 'a' is an assignment to the function name.

What my patch changed (fixed) was how bindingsAccessedDynamically propagates.  Thus, the incorrect deoptimization gets propagated up into the big function where we spend 99% of the time.
Whiteboard: [js:p1]
Attached patch fixSplinter Review
This fix puts the heavyweightness-setting where it should have been put in the first place (where we already set heavyweightness).

This should avoid unnecessarily deoptimizing any function of the forms:
  function f(f) {}
  function f() { var f }

This patch passes shell tests which I verified do test setting the callee.
Assignee: general → luke
Attachment #654044 - Flags: review?(jorendorff)
Nice find Alon, thanks for tracking it down!
Thank you for the quick turnaround! :)
Comment on attachment 654044 [details] [diff] [review]

Review of attachment 654044 [details] [diff] [review]:

Faster, and nicer code, too.

It's still silly to be able to hurt perf by doing such a minor, silly thing. I hope we can make this nicer in the future. If the scope containing the function's own name is basically a let-scope with a const binding, that's something we're going to have to work on for ES6 anyway...
Attachment #654044 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> I hope we can make this nicer in the future.

Agreed!  I guess the closest decomposition for
 (function f() { ... })
would be
 (let (f) (f = function() { ... }))
which fails since we need 'f' to be const.  However, since we're the emitter, it seems like we could just make the binding const and emit JSOP_SETLOCAL/JSOP_SETALIASEDVAR (which doesn't check const-ness) for the init.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.