Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Depends on: 1 bug)

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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(-)
Assignee: general → bhackett1024
(Assignee)

Comment 2

6 years ago
Created attachment 583706 [details] [diff] [review]
WIP (0dd1fb593a49)

Seems to be working in the shell, needs a tryserver run.
Attachment #583555 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
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(-)
Attachment #583706 - Attachment is obsolete: true
Attachment #584066 - Flags: review?(luke)

Comment 4

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

Updated

6 years ago
Attachment #584066 - Flags: review?(dvander)

Comment 5

6 years ago
(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+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d17e22a223
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
(Assignee)

Comment 11

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

Comment 13

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

Comment 15

6 years ago
I've compared v8bench in x86 before/after, and there is no regression in either the shell or browser.
(Assignee)

Comment 16

6 years ago
Backout the backout.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab4f1ebc7cc
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
Last Resolved: 6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 716713
Depends on: 716733

Updated

5 years ago
Depends on: 717184

Updated

5 years ago
Depends on: 717319

Updated

5 years ago
Depends on: 717208
Depends on: 717249
Depends on: 717251
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.
Depends on: 717716
(Assignee)

Comment 20

5 years ago
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: 718076
Depends on: 718823
Depends on: 718347
Depends on: 732669

Updated

5 years ago
Depends on: 732693
Depends on: 717568

Updated

5 years ago
Depends on: 777643
You need to log in before you can comment on or make changes to this bug.