Last Comment Bug 784550 - 2x slowdown on emscripten memops benchmark from bug 775323
: 2x slowdown on emscripten memops benchmark from bug 775323
Status: RESOLVED FIXED
[js:p1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 775323
  Show dependency treegraph
 
Reported: 2012-08-21 16:59 PDT by Alon Zakai (:azakai)
Modified: 2012-08-23 03:50 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
memops benchmark (68.59 KB, application/javascript)
2012-08-21 16:59 PDT, Alon Zakai (:azakai)
no flags Details
fix (2.68 KB, patch)
2012-08-21 18:27 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Alon Zakai (:azakai) 2012-08-21 16:59:26 PDT
Created attachment 654021 [details]
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

http://hg.mozilla.org/mozilla-central/rev/48cfc16cac71

changeset:   102511:48cfc16cac71
user:        Luke Wagner <luke@mozilla.com>
date:        Mon Jul 30 11:38:53 2012 -0700
summary:     Bug 775323 - build Bindings after, not during, parsing (r=ejpbruel)
Comment 1 Luke Wagner [:luke] 2012-08-21 18:01:43 PDT
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.
Comment 2 Luke Wagner [:luke] 2012-08-21 18:27:15 PDT
Created attachment 654044 [details] [diff] [review]
fix

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.
Comment 3 Luke Wagner [:luke] 2012-08-21 18:28:59 PDT
Nice find Alon, thanks for tracking it down!
Comment 4 Alon Zakai (:azakai) 2012-08-21 18:33:04 PDT
Thank you for the quick turnaround! :)
Comment 5 Jason Orendorff [:jorendorff] 2012-08-22 07:46:39 PDT
Comment on attachment 654044 [details] [diff] [review]
fix

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...
Comment 6 Luke Wagner [:luke] 2012-08-22 08:35:04 PDT
(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.
Comment 8 Ed Morley [:emorley] 2012-08-23 03:50:46 PDT
https://hg.mozilla.org/mozilla-central/rev/2a040b73f268

Note You need to log in before you can comment on or make changes to this bug.