Last Comment Bug 738525 - Add IC for JSNative getters
: Add IC for JSNative getters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 749698
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-03-22 18:53 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-05-08 08:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (10.65 KB, patch)
2012-03-23 13:30 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
WIP v2 (10.64 KB, patch)
2012-03-28 10:07 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (14.37 KB, patch)
2012-04-16 17:40 PDT, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review
Patch (15.54 KB, patch)
2012-05-07 09:23 PDT, Jan de Mooij [:jandem]
jdemooij: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-03-22 18:53:51 PDT
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] 2012-03-23 13:30:33 PDT
Created attachment 608848 [details] [diff] [review]
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] 2012-03-28 10:07:29 PDT
Created attachment 610178 [details] [diff] [review]
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] 2012-03-28 10:11:33 PDT
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 (⌚ UTC+1/+2) 2012-03-29 03:13:32 PDT
s/JS_TRUE/true/g, s/JS_FALSE/false/g, please
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-04-13 14:27:50 PDT
Jan, is there a chance this will go in before we branch on the 24th?
Comment 6 Jan de Mooij [:jandem] 2012-04-14 06:57:15 PDT
(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] 2012-04-16 17:40:27 PDT
Created attachment 615565 [details] [diff] [review]
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] 2012-04-16 18:09:46 PDT
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] 2012-04-21 09:21:15 PDT
bhackett: review ping.
Comment 10 Brian Hackett (:bhackett) 2012-04-22 09:35:37 PDT
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] 2012-05-07 09:23:34 PDT
Created attachment 621629 [details] [diff] [review]
Patch

Addresses review comments, still green on try. Going to land this when inbound reopens.
Comment 13 Ed Morley [:emorley] 2012-05-08 03:12:03 PDT
https://hg.mozilla.org/mozilla-central/rev/e763ef9f3d5d

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