Closed
Bug 738525
Opened 12 years ago
Closed 12 years ago
Add IC for JSNative getters
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: bzbarsky, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
15.54 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 4•12 years ago
|
||
s/JS_TRUE/true/g, s/JS_FALSE/false/g, please
Reporter | ||
Updated•12 years ago
|
Blocks: ParisBindings
Reporter | ||
Comment 5•12 years ago
|
||
Jan, is there a chance this will go in before we branch on the 24th?
Assignee | ||
Comment 6•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
bhackett: review ping.
Comment 10•12 years ago
|
||
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 | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e763ef9f3d5d
Target Milestone: --- → mozilla15
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e763ef9f3d5d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
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.
Description
•