574697 - JM: eagerly calculate |this| when the front-end says to

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

We could compute |this| eagerly iff the front-end determines that |this| is used in the method body. For JM we'd flag a bit on the script so that the methodjit compiler emits an eager |this| computation on activation and stores the value in the frame, as in bug 556277, eliding the check emitted by mjit::Compiler::jsop_this() and permitting us to inline further references to the |this| object.

I'm not sure whether this front-end analysis will avoid the issue with XPC_WN_JSOp_ThisObject in bug 556277, but we should think about how to get around that because the benchmark wins are tantalizing ( https://bugzilla.mozilla.org/show_bug.cgi?id=556277#c1 ).

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Found XPC_WN_JSOp_ThisObject in the code base and I think I understand the issue, and why it doesn't apply.

In bug 556227 we had to make it interpreter invariant that |this| was computed for the current frame, so calls to C++ code were also having their thisOp invoked.

Contrastingly, in the JM scheme we can eagerly compute |this| in a prologue for a single method which is understood to contain multiple uses of |this| (therefore likely to pay off).

In the proposal I made above, we're not changing the calling convention to say that |this| will always be available -- theoretically CSEing the computation of |this| and hoisting it to the top of the method so you know it's available in the body.

I'm still reading through http://hg.mozilla.org/tracemonkey/rev/52be13ea0488 pondering what the positive implications are for doing the calculation from the caller instead of the callee.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Eager-this ported to js::Value API.

WIP patch. As Jason pointed out, keeping the eager this invariant makes everything simpler. Now need to update jsop_callname in mjit::Compiler via NameOp to also maintain the same invariant.

Jason also landed a patch on TM recently to cache the JSObjects created for global classes -- I think he said that would avoid the window object creation overhead problem from the referenced bug.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Eager-this ported to js::Value API with JM support.

No win on my desktop for either sunspider or v8, but the number of JSOP_THIS slowcalls dropped to zero. Now hoisting computation into header without front-end analysis to see what that does.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Eager-this ported to js::Value API with JM support.

When I said no win, I meant fairly epic win. 1.1s on v8 on my macbook -- was accidentally running with "-j -m", and something weird was happening where the tracer was activating. I guess we can conclude we eliminated ~100 cycles of overhead for each of the 33e6 slow calls.

I figure computed-this was an unpredictable branch and that there's spill/fill overhead and loss of I$ locality for the slow call -- not sure if that quite adds up. I kind of want to count the number of |this| computations and see if they were reduced in number somehow that I'm not thinking of. In any case, I'm off for a victory burrito.

Outstanding work:
- make check shows embedder API is getting a null function value in js_Construct hook invocation.
- tracer support for the new invariant needs to be ported.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 454755 [details] [diff] [review]
Eager-this ported to js::Value API with JM support.

Awesome.

>         /* Primitive |this| should not be passed to slow natives. */
>-        JSObject *thisp = &fp->thisv.asObject();
>+        JSObject *thisp = fun ? fp->getThisObject(cx) : NULL;

Just a note that we need to check this with Jason for the jsapi-test failure.

>     if OPTIONS.show_cmd:
>-        print(cmd)
>+        print(subprocess.list2cmdline(cmd))

Remember not to check this in - separate bug.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/e30feb436f0e

From callgrind it appears that we really were saving about a hundred instructions per slowcall, which microbenchmarks support. Unhelpfully, js::ComputeThisFromArgv wasn't getting inlined into the slow call body, so that's a nice thing to force inline in the follow up patch.

In the new scheme this is what happens, step by step, for the interested:

function Duck() { this.hitCount = 0; }
Duck.prototype.punch() { this.hitCount++; }
(function() {
    var d = new Duck();
    for (var i = 0; i < 33e6; i++) d.punch();
})();

- First setting of this: ExecuteScript => Execute => pushes the primordial frame. |this| is the result of calling the thisObject hook on the scope chain object. The scope chain object and the result are the global passed in from the shell.
- Second setting of |this| is a NullTag() given during the anonymous function's invocation. This indicates that |this| should be resolved on-demand by the callee as a global.
- Third setting of |this| is under the |new| slowcall, which generates a new object to be used as |this| and clobbers the null value that the bytecode stream places on the stack. This is our boxing duck.
- Fourth setting of |this| is under the slowcall. The bytecode sequence looks thusly:

[jaeger] JSOps         0 00021:  10  getlocal 0
[jaeger] JSOps         1 00024:  10  callprop "punch"
[jaeger] JSOps         2 00027:  10  call 0

And that call (27) sets the newly pushed frame's JSStackFrame::thisv to to the getlocal result, which callprop guarantees isObject as a postcondition.
- All subsequent calls to |punch| perform the same getlocal-result-to-pushed-frame-thisv movement under a fastcall.

Next up: a patch ensuring that |this| is computed in the prologue of methods that the front-end determines to require it. This approach turns JSOP_THIS into a single move instruction -- there is no longer a slow call guard on the NullTag() value, because thisv is never null in a methods that needs it.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Eager this computation in prologue.

Hoist computation of eager this in the prologue by flagging JSOP_THIS usage in the parser.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 455054 [details] [diff] [review]
Eager this computation in prologue.

No perf change on my machine. Maybe we should investigate eliminating the type load.

In jsop_this(), if !script->strictModeCode, I *think* we should be able to predict thisv as always being JSVAL_TAG_OBJECT.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: JM eager this tracer changes.

Stolen from jorendorff's patch in the referenced bug.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> >         /* Primitive |this| should not be passed to slow natives. */
> >-        JSObject *thisp = &fp->thisv.asObject();
> >+        JSObject *thisp = fun ? fp->getThisObject(cx) : NULL;
> 
> Just a note that we need to check this with Jason for the jsapi-test failure.

...What failure?

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #10)
> ...What failure?

There was some jsapi-test that was failing because of an initialization value of |this|... sorry I don't remember any more details than that, it seems to have disappeared. Maybe dvander remembers?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

It was a trace test where .call (I think) had changed from a slow to a fast native, and this changeset disabled it asserting:

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/39d35f51ea51

Comment 13

[Brendan Eich [:brendan]]

Global frames should get the right non-null |this| eagerly, no?

/be

Comment 14

[David Mandelin [:dmandelin]]

Comment on attachment 462311 [details] [diff] [review]
JM eager this tracer changes.

This patch needs rebasing. Also, it should live in a separate bug. I'm going to move it to bug 587809, as it fixes that bug.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

Okay, I'm going to mark this as fixed since we broke out the other bug for the tracer changes.