The default bug view has changed. See this FAQ.

2x slowdown on emscripten memops benchmark from bug 775323

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: azakai, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p1])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Updated

5 years ago
Blocks: 775323
(Assignee)

Comment 1

5 years ago
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]
(Assignee)

Comment 2

5 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #654044 - Flags: review?(jorendorff)
(Assignee)

Comment 3

5 years ago
Nice find Alon, thanks for tracking it down!
(Reporter)

Comment 4

5 years ago
Thank you for the quick turnaround! :)
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...
Attachment #654044 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a040b73f268
https://hg.mozilla.org/mozilla-central/rev/2a040b73f268
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.