Closed
Bug 712714
Opened 12 years ago
Closed 12 years ago
Remove JOF_CALLOP
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file, 2 obsolete files)
229.54 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
Seems to be working in the shell, needs a tryserver run.
Attachment #583555 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
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•12 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•12 years ago
|
Attachment #584066 -
Flags: review?(dvander)
Comment 5•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d17e22a223
Comment 8•12 years ago
|
||
This seems to have regressed tons of Talos benchmarks...
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 12•12 years ago
|
||
(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•12 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.
Comment 14•12 years ago
|
||
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•12 years ago
|
||
I've compared v8bench in x86 before/after, and there is no regression in either the shell or browser.
Assignee | ||
Comment 16•12 years ago
|
||
Backout the backout. https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab4f1ebc7cc
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•