The default bug view has changed. See this FAQ.

Add IC for JSNative getters

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
Attachment #608848 - Attachment is obsolete: true

Comment 3

5 years ago
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.
s/JS_TRUE/true/g, s/JS_FALSE/false/g, please
Blocks: 580070
Jan, is there a chance this will go in before we branch on the 24th?
(Assignee)

Comment 6

5 years ago
(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
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
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?).
Assignee: general → jdemooij
Attachment #610178 - Attachment is obsolete: true
Attachment #615565 - Flags: review?(bhackett1024)

Comment 8

5 years ago
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'?
(Assignee)

Comment 9

5 years ago
bhackett: review ping.
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?
Attachment #615565 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Depends on: 749698
(Assignee)

Comment 11

5 years ago
Created attachment 621629 [details] [diff] [review]
Patch

Addresses review comments, still green on try. Going to land this when inbound reopens.
Attachment #615565 - Attachment is obsolete: true
Attachment #621629 - Flags: review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e763ef9f3d5d
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/e763ef9f3d5d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Add ICs for accessor properties whose getter/setter are JSNatives → Add IC for JSNative getters
You need to log in before you can comment on or make changes to this bug.