Last Comment Bug 712714 - Remove JOF_CALLOP
: Remove JOF_CALLOP
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 717568 716713 716733 717184 717208 717249 717251 717319 717716 718076 718347 718823 732669 732693 777643
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-21 10:47 PST by Brian Hackett (:bhackett)
Modified: 2012-08-21 15:48 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (0dd1fb593a49) (214.53 KB, patch)
2011-12-21 10:50 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Review
WIP (0dd1fb593a49) (247.06 KB, patch)
2011-12-21 19:13 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Review
patch (0dd1fb593a49) (229.54 KB, patch)
2011-12-23 08:17 PST, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2011-12-21 10:47:49 PST
JOF_CALLOP instructions like CALLLOCAL, CALLPROP etc. push two values --- the actual accessed name/property, and the 'this' value used for the call.  These two values are computed independently from one another.  The 'this' value is exactly the lvalue for PROP and ELEM, and is undefined or an implicit 'this' computed from a scope object otherwise.  There is an exception where the function value influences the result of ComputeImplicitThis, but only in cases where the function is primitive and we're about to throw anyways; this looks like a leftover from previous logic and there shouldn't be a problem in removing this special case.

It would be nicer if the function value and 'this' value were computed by different opcodes.

Instead of:     Do:

CALLLOCAL x     CALLOCAL x
                UNDEFINED

GETLOCAL x      GETLOCAL x
CALLPROP f      DUP
                CALLPROP f
                SWAP

CALLNAME x      CALLNAME x
                IMPLICITTHIS x

The CALL ops stick around mainly for the decompiler.  CALLLOCAL is identical to GETLOCAL, and CALLPROP is identical to GETPROP except in cases where the call context influences behavior (OnUnknownMethod, js_GetXMLMethod).

IMPLICITTHIS also needs to perform a name lookup, which would increase the amount of time to resolve such calls.  The only times when IMPLICITTHIS can push anything other than undefined are inside 'with' blocks and when running against a non-global scope object (which can only happen in non-compileAndGo code).  These situations are very rare and it should be fine to just determine at parse time whether a CALLNAME needs an IMPLICITTHIS or an UNDEFINED, and then stub encountered IMPLICITTHIS during compilation.

This is necessary to cleanly handle call opcodes in IonMonkey.  IonMonkey associates each stack operand with the instruction that pushed it, so it's kind of a no-go for a single instruction to push two operands.  The operation can't easily be split into two instructions, either, as a rejoin after the first one (as in a CALLPROP or CALLNAME, say) would not correspond to any clean bytecode boundary.  (JM can rejoin into the middle of an opcode, but the associated code to reconstruct a valid interpreter state is pretty nasty and it would be great to totally avoid this in IM).
Comment 1 Brian Hackett (:bhackett) 2011-12-21 10:50:25 PST
Created attachment 583555 [details] [diff] [review]
WIP (0dd1fb593a49)

WIP, mostly works in the interpreter.  This does a lot of cleanup and code removal around the opcodes too, to combine interpreter cases and ICs.  This also removes the property cache, to make this cleanup easier.  Still needs more work before perf can be tested with this, but I'm hoping to not need to add the property cache back.

 29 files changed, 447 insertions(+), 2320 deletions(-)
Comment 2 Brian Hackett (:bhackett) 2011-12-21 19:13:36 PST
Created attachment 583706 [details] [diff] [review]
WIP (0dd1fb593a49)

Seems to be working in the shell, needs a tryserver run.
Comment 3 Brian Hackett (:bhackett) 2011-12-23 08:17:50 PST
Created attachment 584066 [details] [diff] [review]
patch (0dd1fb593a49)

Working patch.  This no longer removes the property cache, which is still necessary for performance in SS and other cases where property accesses are stubbed.  Uses of the property cache are cleaned up a good deal, though, with code shared between the interpreter and JM.  I'm not sure yet what the right role for the property cache should be in IM; the cache uses a pc, but does not access it and it is essentially an arbitrary word which determines the type of access being performed and atom being accessed.

 31 files changed, 806 insertions(+), 2176 deletions(-)
Comment 4 Luke Wagner [:luke] 2011-12-23 10:28:09 PST
Comment on attachment 584066 [details] [diff] [review]
patch (0dd1fb593a49)

I like those diffstats, but I don't think I'm the right person for this patch.  Maybe dvander?
Comment 5 Luke Wagner [:luke] 2011-12-23 15:39:49 PST
(In reply to Brian Hackett (:bhackett) from comment #3)
> Created attachment 584066 [details] [diff] [review]
> patch (0dd1fb593a49)
> 
> Working patch.  This no longer removes the property cache, which is still
> necessary for performance in SS and other cases where property accesses are
> stubbed.

OOC, which stubs (GetProp, GetElem, Name)?
Comment 6 David Anderson [:dvander] 2012-01-04 17:08:34 PST
Comment on attachment 584066 [details] [diff] [review]
patch (0dd1fb593a49)

Review of attachment 584066 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2760,5 @@
> +        return true;
> +    if (!inFunction()) {
> +        JSObject *scope = scopeChain();
> +        while (scope) {
> +            if (scope->isWith())

Should this be |if (!obj->isGlobal() && !IsCacheableNonGlobalScope(obj))| or does compileAndGo guarantee that With scopes are the only oddity?
Comment 7 Brian Hackett (:bhackett) 2012-01-05 11:09:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d17e22a223
Comment 8 :Ehsan Akhgari (out sick) 2012-01-05 16:53:24 PST
This seems to have regressed tons of Talos benchmarks...
Comment 9 Marco Bonardo [::mak] 2012-01-06 03:35:07 PST
plus a bunch of changes to the same files landed, making the backout not that trivial, unless we backout all js changesets landed after this one.
Comment 10 Marco Bonardo [::mak] 2012-01-06 04:51:52 PST
I backed out the following changesets to try bringing back the tree to a mergeable status.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d337401801
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d17e22a223
Comment 11 Brian Hackett (:bhackett) 2012-01-06 05:09:38 PST
The only thing that regressed is V8, and I can fix whatever needs to be fixed in place.  But this backout is screwing up JS development that needs to happen on top of this patch.  I posted a message in this effect to dev-tree-management.  Please do not back out JS patches which regress JS tests without getting an OK from whoever wrote the patch.
Comment 12 Marco Bonardo [::mak] 2012-01-06 05:14:18 PST
(In reply to Brian Hackett (:bhackett) from comment #11)
> Please do not back out JS patches which regress JS
> tests without getting an OK from whoever wrote the patch.

This is clearly against the general trees rules, since would keep the trees closed  while we wait for an answer, the alternatives are backout or close the tree.
Btw, I honestly don't see any message in tree-management, apart the answer to my message.
Comment 13 Brian Hackett (:bhackett) 2012-01-06 05:31:48 PST
(In reply to Marco Bonardo [:mak] from comment #12)
> This is clearly against the general trees rules, since would keep the trees
> closed  while we wait for an answer, the alternatives are backout or close
> the tree.

This is an absurd reaction to a patch which only regresses performance metrics.

> Btw, I honestly don't see any message in tree-management, apart the answer
> to my message.

Sorry, the message I sent was not reply-all.
Comment 14 Marco Bonardo [::mak] 2012-01-06 05:41:55 PST
Sorry, my intent was clearly not to create you issue, but to keep the tree sane for hundreds of developers. As far as I remember perf regressions have always been backed out promptly to avoid building on top of them thus making them unremovable at a later stage, and this system worked out pretty well.

If the JS team needs special rules regarding perf regressions, please suggest them in tree-management, so that we can add those to the tree rules and all sheriffs will be aware of them.
Comment 15 Brian Hackett (:bhackett) 2012-01-06 18:42:56 PST
I've compared v8bench in x86 before/after, and there is no regression in either the shell or browser.
Comment 16 Brian Hackett (:bhackett) 2012-01-09 06:31:56 PST
Backout the backout.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab4f1ebc7cc
Comment 17 Marco Bonardo [::mak] 2012-01-09 12:07:10 PST
Should we file a bug about the difference between what you measure and what Talos measures? 10% is an high difference and if it's fake, the test has a low level of reliability.
Comment 18 Ed Morley [:emorley] 2012-01-09 15:03:49 PST
https://hg.mozilla.org/mozilla-central/rev/7ab4f1ebc7cc
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 10:03:46 PST
One of the fixes for regressions from this bug also made jsfiddle work again (the patch in this bug had broken it).

Can we add testcases to exercise whatever was involved in those regressions, please?  Clearly we don't have them now, and the web relies on it.
Comment 20 Brian Hackett (:bhackett) 2012-01-13 03:29:57 PST
There is a testcase for bug 717208, but not bug 717184.  The latter is a codegen bug which would be pretty hard to tickle, and doubly so in the shell as the bug requires use of getter hooks which are almost non-existent outside the DOM.

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