Last Comment Bug 753158 - emit ALIASEDVAR ops for upvars
: emit ALIASEDVAR ops for upvars
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal with 2 votes (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on: 659577 753145 765956 773609 773844 775807 776191
Blocks: IonSpeed 765980 768743 772688
  Show dependency treegraph
Reported: 2012-05-08 15:40 PDT by Luke Wagner [:luke]
Modified: 2012-07-26 01:13 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP 1 (11.06 KB, patch)
2012-07-04 23:14 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
WIP 2 (48.23 KB, patch)
2012-07-06 03:48 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (37.15 KB, patch)
2012-07-07 16:12 PDT, Luke Wagner [:luke]
bhackett1024: review+
dvander: feedback+
Details | Diff | Splinter Review
fix and test (2.51 KB, patch)
2012-07-16 17:11 PDT, Luke Wagner [:luke]
dvander: review+
Details | Diff | Splinter Review

Description User image Luke Wagner [:luke] 2012-05-08 15:40:27 PDT
Currently, the ALIASEDVAR ops are only emitted for local accesses.  Once we have script scope nesting information (bug 753145), we can emit ALIASEDVAR ops for upvar accesses as well.  (The nesting information is necessary for TI to connect the typesets of upvar reads/writes.)  This will be much faster than using JSOP_NAME.  Hopefully, we can measure and show that ALIASEDVAR makes the non-reentrant closure optimization (and all of the associated TypeScriptNesting stuff) unnecessary.
Comment 1 User image David Anderson [:dvander] 2012-06-25 18:11:04 PDT
Looks like this might be about 3-4% on v8, based on changing MGetNameCache to be an optimizable instruction.
Comment 2 User image Luke Wagner [:luke] 2012-07-04 23:14:21 PDT
Created attachment 639232 [details] [diff] [review]

This patch takes care of the VM, just need to fix up the now-broken use of ScopeCoordinateToFrameIndex in JM/TI.  Pretty close, though.
Comment 3 User image Luke Wagner [:luke] 2012-07-06 03:48:25 PDT
Created attachment 639620 [details] [diff] [review]

This one passes tests.  On a microbenchmark, it is about 20% faster accessing a single upvar and it takes the earley-boyer score from roughly 11000 to 12000.

However, it is actually a bit slower on navier-stokes due to the fact that 100% of typesets are "unknown" (compared to 100% having int32 w/ the non-reentrant closure optimization).  To fix this, I need to use addSubsetBarrier (instead of just addSubset), make the ALIASEDVAR ops have JOF_TYPESET, and add the barrier to jit access paths.
Comment 4 User image Luke Wagner [:luke] 2012-07-07 16:12:41 PDT
Created attachment 640004 [details] [diff] [review]

This patch adds a type barrier which recovers type info.  With this change on JM+TI, I see the same perf as when the non-reentrant closure optimization applies (both in a micro-bench and on v8/navier-stokes).  In a micro-bench where the non-reentrant closure doesn't apply (e.g., doing y += x where both are upvars), the patch is about 2x faster.  Lastly, on JM+TI, our earley-boyer score, measured in the shell, is about 7% better.

Flagging Brian for review to make sure I got the TI/jit stuff right.  Flagging David for feedback mostly so you can see changes necessary for IM (note the added type barrier to the GETALIASEDVAR path, see comment in jsinfer.cpp).

(Patch is green on try.)
Comment 5 User image Brian Hackett (:bhackett) 2012-07-07 16:31:00 PDT
Comment on attachment 640004 [details] [diff] [review]

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


::: js/src/jsanalyze.cpp
@@ +319,5 @@
>              break;
>            case JSOP_GETALIASEDVAR:
>            case JSOP_CALLALIASEDVAR:
> +          case JSOP_SETALIASEDVAR:

Can you fold these cases in with the previous one, since the case bodies are the same?
Comment 7 User image Ed Morley [:emorley] 2012-07-12 05:24:06 PDT
Unfortunately bug 772303 ( had to be backed out for winxp pgo-only jsreftest failures.

This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. 

Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out:
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2012-07-12 18:01:12 PDT
Comment 10 User image Ryan VanderMeulen [:RyanVM] 2012-07-13 20:49:45 PDT
Backed out due to bug 773609 and bug 773844.
Comment 11 User image Luke Wagner [:luke] 2012-07-16 17:11:43 PDT
Created attachment 642798 [details] [diff] [review]
fix and test

Arg, there is a pre-existing bug where the parser completely ignores function statements when it links uses to definitions in the enclosing scope.  The fix is  to just tweak the existing deoptimization check in LeaveFunction so that it catches function statements in addition to eval/with.
Comment 12 User image Luke Wagner [:luke] 2012-07-16 17:12:19 PDT
(This patch fixes both two regressions in comment 10.)
Comment 15 User image steve 2012-07-22 14:42:06 PDT
On Maemo5 with XUL/Qt build (ARM Thumb2/NEON), we've noticed a significant slowdown in our Sunspider benchmark performance.

Prior to this commit we achived 6060ms on N900 at stock CPU frequency.

After this commit it was down to 6734ms.

Current mozilla-central gives 8900ms, so we've got more regressions to find. :(
Comment 16 User image Luke Wagner [:luke] 2012-07-23 09:34:59 PDT
After this patch landed, we had to fix the string.replace(lambda) hack: bug 775801.  Do you still see a regression with this fix?
Comment 17 User image Ivaylo Dimitrov 2012-07-26 01:13:29 PDT
With latest m-c the sunspider result is 6120ms.

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