647626 - Remove compound GET*PROP opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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 [Laid off!]]

Attachment: 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 [Laid off!]]

Er, forgot to change JSXDR_BYTECODE_VERSION, will fix that when this is ready to push.

Comment 3

[David Mandelin [:dmandelin]]

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]

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 [Laid off!]]

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 [Laid off!]]

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 [Laid off!]]

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]

> 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]]

(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