emit ALIASEDVAR ops for upvars

RESOLVED FIXED in mozilla17



7 years ago
7 years ago


(Reporter: luke, Assigned: luke)


(Blocks: 2 bugs)

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:t])


(2 attachments, 2 obsolete attachments)



7 years ago
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.
Whiteboard: [js:t]


7 years ago
Depends on: 765956
Looks like this might be about 3-4% on v8, based on changing MGetNameCache to be an optimizable instruction.

Comment 2

7 years ago
Posted patch WIP 1 (obsolete) — Splinter 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.
Assignee: general → luke

Comment 3

7 years ago
Posted patch WIP 2 (obsolete) — Splinter 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.
Attachment #639232 - Attachment is obsolete: true

Comment 4

7 years ago
Posted patch patchSplinter 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.)
Attachment #639620 - Attachment is obsolete: true
Attachment #640004 - Flags: review?(bhackett1024)
Attachment #640004 - Flags: feedback?(dvander)
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?
Attachment #640004 - Flags: review?(bhackett1024) → review+
Attachment #640004 - Flags: feedback?(dvander) → feedback+


7 years ago
Blocks: 772688

Comment 6

7 years ago
Target Milestone: --- → mozilla16
Unfortunately bug 772303 (https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5) 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:

Target Milestone: mozilla16 → ---


7 years ago
Target Milestone: --- → mozilla16
Last Resolved: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED


7 years ago
Depends on: 773609


7 years ago
Depends on: 773844
Backed out due to bug 773609 and bug 773844.
Resolution: FIXED → ---
Target Milestone: mozilla16 → ---

Comment 11

7 years ago
Posted patch fix and testSplinter Review
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.
Attachment #642798 - Flags: review?(dvander)

Comment 12

7 years ago
(This patch fixes both two regressions in comment 10.)
Attachment #642798 - Flags: review?(dvander) → review+
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 775807
Depends on: 776191

Comment 15

7 years ago
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

7 years ago
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

7 years ago
With latest m-c the sunspider result is 6120ms.
You need to log in before you can comment on or make changes to this bug.