Closed Bug 712714 Opened 12 years ago Closed 12 years ago

Remove JOF_CALLOP

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attached patch WIP (0dd1fb593a49) (obsolete) — Splinter Review
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(-)
Assignee: general → bhackett1024
Attached patch WIP (0dd1fb593a49) (obsolete) — Splinter Review
Seems to be working in the shell, needs a tryserver run.
Attachment #583555 - Attachment is obsolete: true
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(-)
Attachment #583706 - Attachment is obsolete: true
Attachment #584066 - Flags: review?(luke)
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?
Attachment #584066 - Flags: review?(luke)
Attachment #584066 - Flags: review?(dvander)
(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 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?
Attachment #584066 - Flags: review?(dvander) → review+
This seems to have regressed tons of Talos benchmarks...
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.
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
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.
(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.
(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.
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.
I've compared v8bench in x86 before/after, and there is no regression in either the shell or browser.
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.
https://hg.mozilla.org/mozilla-central/rev/7ab4f1ebc7cc
Status: NEW → RESOLVED
Closed: 12 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 717184
Depends on: 717319
Depends on: 717208
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.
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.
Depends on: 732669
Depends on: 732693
Depends on: 717568
Depends on: 777643
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: