738525 - Add IC for JSNative getters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We currently have an IC for getters when the shape has a pointer to a JSPropertyOp.

But in the new DOM bindings we tried to use JSPROP_NATIVE_ACCESSORS with JSNatives backing the function objects for our properties, because that makes things like __lookupGetter__ and __lookupSetter__ just work and also makes bug 520882 not matter (and in particular, makes bug 560072 go away).

I've now seen several people run into bug 560072 in the wild...

The problem is that using JSPROP_NATIVE_ACCESSORS and JSNatives means we don't have an IC, and then property access costs are much higher.  For a simple getter invocation, the new DOM bindings are at about 15ns of overhead without JSPROP_NATIVE_ACCESSORS and at about 90ns of overhead with that flag.  For comparison, quickstubbed things range from about 20ns to 65ns on the same hardware depending on the type of the |this| object.

It seems like morally dealing with function accessors which are backed by a JSNative should not cause any more difficulty than calling a JSPropertyOp, right?  Or are there complications past "we just haven't written the masm goop to make this work"?

For now I'm going to duplicate bug 462428 in the new bindings to make lookupGetter and lookupSetter work, but the property descriptor stuff would still be broken as things stand.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP

bz asked me about this on IRC and we came up with this patch. This even seems to work, kind of, but there's still more work to do (a bunch of debugger jit-tests fail for instance).

I'm also not sure if the stack is guaranteed to have room for two Value's etc.

Performance is a bit slower than the PropertyOp getter, I'm not sure why; there's an extra store to vp, but I don't know if that's the only reason.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP v2

Rebased and passes jit-tests now - the Debugger object also uses JSNative getters and this exposed a bug.

There's still a problem with the stack pointer, we push an extra value on the stack so we should either update sp (may cause StackIter to complain) or store these two values somewhere else.

Comment 3

[Luke Wagner [:luke]]

So, 'yes': if you put values onto the top of the stack you definitely need to bump 'sp' so that your values don't get clobbered if the callee reenters the VM.  Also, StackIter isn't just 'complaining', it loads from sp (to determine whether there has been a native call) which means it could crash if sp is wrong.  However, this is only when *pc == JSOP_CALL or JSOP_FUNCALL, so you should be fine here.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

s/JS_TRUE/true/g, s/JS_FALSE/false/g, please

Comment 5

[Boris Zbarsky [:bzbarsky]]

Jan, is there a chance this will go in before we branch on the 24th?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Jan, is there a chance this will go in before we branch on the 24th?

Yes, I pushed an updated patch to try:

https://tbpl.mozilla.org/?tree=Try&rev=96bb268f279f

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Adds the "JSNative-getter" IC, it's similar to the "JSPropertyOp-getter" IC.

This also changes the JSPropertyOp IC to bump sp - it uses vp == sp so if the getter calls back into JS it could clobber *vp. Brian, can you confirm it's okay to use 2 extra slots (and check the comment?).

Comment 8

[Luke Wagner [:luke]]

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

>+             * for loop temporaries.
>+             */
>             state_ = SCRIPTED;
>             script_ = fp_->script();
>+            if (op == JSOP_GETPROP || op == JSOP_CALLPROP)
>+                JS_ASSERT(sp_ >= fp_->base() && sp_ <= fp_->slots() + script_->nslots + 2);

super nit (and nice comment btw), but could you put the comment after the two assignments, before the 'if'?

Comment 9

[Jan de Mooij [:jandem]]

bhackett: review ping.

Comment 10

[Brian Hackett [Laid off!]]

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

Review of attachment 615565 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: js/src/shell/js.cpp
@@ +4183,5 @@
> +    JS_SetPrivate(obj, (void *)val);
> +
> +    *val = *argv;
> +    return true;
> +}

Can you add a testcase to exercise these?

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch

Addresses review comments, still green on try. Going to land this when inbound reopens.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e763ef9f3d5d

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e763ef9f3d5d