Last Comment Bug 647626 - Remove compound GET*PROP opcodes
: Remove compound GET*PROP opcodes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks: 647620
  Show dependency treegraph
 
Reported: 2011-04-03 18:51 PDT by Brian Hackett (:bhackett)
Modified: 2011-10-17 15:14 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (46.28 KB, patch)
2011-04-19 14:26 PDT, Brian Hackett (:bhackett)
dmandelin: review+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2011-04-03 18:51:22 PDT
The GETTHISPROP, GETARGPROP, and GETLOCALPROP opcodes make JIT and analysis implementation more complex, and don't do anything to improve the generated JIT code.
Comment 1 Brian Hackett (:bhackett) 2011-04-19 14:26:21 PDT
Created attachment 527105 [details] [diff] [review]
patch

Remove GET*PROP opcodes.  Also changes the format of JSOP_LENGTH so it has an atom, and is just an annotated JSOP_GETPROP, avoiding some ugly gotos dealing with the different formats of the two ops (could remove it entirely, but that would need more mucking with the interpreter and tracer).

14 files changed, 168 insertions(+), 442 deletions(-)

Need this for SSA in the JM branch (we need a real stack value to track which LOCAL/ARG value is being accessed by the GETLOCALPROP and GETARGPROP), will apply there.
Comment 2 Brian Hackett (:bhackett) 2011-04-19 14:27:39 PDT
Er, forgot to change JSXDR_BYTECODE_VERSION, will fix that when this is ready to push.
Comment 3 David Mandelin [:dmandelin] 2011-04-19 14:52:32 PDT
Comment on attachment 527105 [details] [diff] [review]
patch

The removal would be nice, but feel free to skip the mucking.

I would be nice to land this to TM: does it hurt performance in the standard configuration there? What about pure interp?
Comment 4 Andreas Gal :gal 2011-04-19 14:55:47 PDT
Very interested in the questions in comment 3, and yes, if we can solve all those questions I would love to see this on tip.
Comment 5 Brian Hackett (:bhackett) 2011-04-19 15:04:39 PDT
I'll measure the perf change, but even though this will slowdown the interpreter some (no JIT effect) this change is necessary for the analysis work going on in the JM branch which will feed into substantial JIT optimizations.

I think that except in really egregious cases we really shouldn't worry much what our interpreter performance is at the peephole level. (Bigger changes, like inlining the property cache into the bytecode or using a register based bytecode, are more important.  A register based bytecode would obviate these ops anyways).
Comment 6 Brian Hackett (:bhackett) 2011-04-19 15:14:31 PDT
Interpreter times for GET{ARG|LOCAL|THIS}PROP.

function foo(x) {
  for (var i = 0; i < 1000000; i++) {
    x.f;
  }
}
foo({f:0});

        ARG  LOCAL  THIS

Before: 45   45     48
After:  48   43     46

Yes, the LOCAL and THIS cases got faster (don't ask me why!).  OK to push?
Comment 7 Brian Hackett (:bhackett) 2011-04-20 06:51:39 PDT
Push to JM.

http://hg.mozilla.org/projects/jaegermonkey/rev/fbcbc74151c1

This caused some orange on mochitest-other, extra warnings like:

reference to undefined property this._updateFeedTimeout\" {file: \"chrome://browser/content/browser.js\" line: 12358}

This warnings is reported by js_GetPropertyHelperWithShapeInline, which looks to have been buggy before this patch, with this test:

                if (!cx->hasStrictOption() ||
                    (op != JSOP_GETPROP && op != JSOP_GETELEM) ||
                    js_CurrentPCIsInImacro(cx)) {
                    return JS_TRUE;
                }

Strict warnings for undefined properties were not being generated if the property access was a GET{THIS|ARG|LOCAL}PROP opcode, and still isn't for LENGTH or INC{PROP|ELEM} opcodes (hazards with fragmenting the bytecode...).  I think the behavior is better now and these warnings are legitimate, the offending line looks to be:

      clearTimeout(this._updateFeedTimeout);

Which runs with strict mode on.  Will stick a test around this statement and see if the warnings go away.
Comment 8 Paul Biggar 2011-05-09 03:59:23 PDT
> Which runs with strict mode on.  Will stick a test around this statement and
> see if the warnings go away.

http://hg.mozilla.org/projects/jaegermonkey/rev/03aa9eb2a8fc
Comment 9 Brendan Eich [:brendan] 2011-07-09 00:06:39 PDT
(In reply to comment #6)
> Interpreter times for GET{ARG|LOCAL|THIS}PROP.
> 
> function foo(x) {
>   for (var i = 0; i < 1000000; i++) {
>     x.f;
>   }
> }
> foo({f:0});
> 
>         ARG  LOCAL  THIS
> 
> Before: 45   45     48
> After:  48   43     46
> 
> Yes, the LOCAL and THIS cases got faster (don't ask me why!).  OK to push?

Firefox 3 (no JITs) sped up due to all this bytecode fussing. Glad to see it go.

/be

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