515496 - (blazinglyfastthis) Eliminate extra security check when computing this (in js_ComputeGlobalThis)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, computing this is very expensive, even in the interpreter and tracer. We end up calling into a hook that needs to look at the stack and make all sorts of bizarre decisions. In reality, when we're in the interpreter, we never need to do this. All 'this' computations that happen in the interpreter can use the wrapper for their scope (note that assertion relies on full wrapperization, which is not quite here yet, thanks to evalInSandbox).

The overall proposal is to tie JSCLASS_GLOBAL_FLAGS to a slot on the global object that contains the "stunt this object" and eagerly use that when calling functions.

There is one case where we can't directly use the stunt this object, which is on entry from native code (via the API) in that case, we should be able to simply call the thisObject hook or something to find the right this object to use.

Comment 1

[Jason Orendorff [:jorendorff]]

> All 'this' computations that happen in the interpreter
> can use the wrapper for their scope

Which wrapper? and what does "scope" mean here?

> (note that assertion relies on full wrapperization, which is not quite
> here yet, thanks to evalInSandbox).

How much effort would this be?

In bug 513160 comment 8 I proposed computing `this` eagerly rather than lazily, and caching those computations via the property cache. But certainly if the computation can be made BLAZINGLY FAST, that's even better.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch

So, a couple of things got in the way of really BLAZINGLY fast 'this'. In particular, I was really hoping to avoid having to call any hooks for 'this' wrapping in the common case. This doesn't quite get us there, but it gets rid of the security check that is now unnecessary.

In order to get rid of the wrapper *hook*, we need to group wrappers by origin, not scope as we do now. That patch is coming (for other, more awesome reasons as well).

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 433450 [details] [diff] [review]
Patch

>-         * When the above FIXME is made fixed, the whole GC reachable frame
>-         * mechanism can be removed as well.

I think this is true. The only other use of JSGCReachableFrame is in jsdbgapi.cpp:JS_GetFrameThis, and I think that can be removed.

Otherwise this patch looks great.

Comment 4

[Jason Orendorff [:jorendorff]]

Another note: It seems better to inline JS_GetGlobalForObject or at least avoid the expense of calling the public API. A JSObject method might be nice.

Comment 5

[Jason Orendorff [:jorendorff]]

I pushed this to tm. mrbkap is busy.

http://hg.mozilla.org/tracemonkey/rev/9851a9b1647f

Comment 6

[Jason Orendorff [:jorendorff]]

Adjusting the summary to reflect what actually happened here.

I'm leaving the (blazinglyfastthis) alias for now. I hope that alias can be re-assigned to a followup bug soon!

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9851a9b1647f

Comment 8

[Josh Matthews [:jdm]]

Is JS_THIS_OBJECT infallible now?  Should bug 498543 be closed?