This uses the infrastructure from bug 877216.

Still needs jit changes to pass the right things in

Peter, would you review the codegen changes? Jan, would you review the JS engine changes? As far as the JS engine changes go, please pay careful attention to the visitCallDOMNative bits and ionframe bits in terms of the following: 1) Can we add more asserts somehow to make this whole setup safer? 2) Can we make better use of registers by having another scratch reg to work with? At first glance no, but please double-check that! 3) Is there a cleaner way to compute the argv value to push on the stack? In terms of performance this makes a no-op DOM call slower by about 0.5ns on my machine, presumably for two reasons. First of all, the C++ implementation has to return JS::UndefinedValue(). This used to involve just writing it to *vp, but now it has to effectively be written to *(args->array), which means an extra pointer-chase. Second, the actual jit codegen is a bit different, obviously. The relevant change is the old setup did this in the JIT: ;; movePtr(StackPointer, argVp) movq %rsp, %r8 whereas the new setup does this: ;; computeEffectiveAddress( ;; Address(StackPointer, sizeof(uintptr_t) + 2*sizeof(Value)), ;; argArgs) leaq 0x18(%rsp), %rcx ;; Push(argArgs); push %rcx ;; movePtr(StackPointer, argArgs); movq %rsp, %rcx And I'm on x86-64 so having to pass one less argument to the ABI call doesn't matter: they're all getting passed in registers anyway. It's possible that on x86-32 there is no performance impact and that in a non-microbenchmark the reduced register pressure helps or something...

This is pure search-and-replace

I'm sorry this is so big; I couldn't find a good way to split this up

Comment on attachment 755982 [details] [diff] [review] part 5. Add a set() method to Rooted. Review of attachment 755982 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: js/public/RootingAPI.h @@ +578,5 @@ > ptr = value; > return ptr; > } > > + void set(T value) { Why not |const T &value|?

> Why not |const T &value|? I cribbed from MutableHandle. But a possibly-good reason for not doing that is that this code: Rooted<JSObject*> rooted(cx); JSObject* obj = GetSomeObject(); rooted.set(obj); would end up with an unsafe reference hazard if the set() argument is a const T&....

Comment on attachment 755973 [details] [diff] [review] part 1. Convert WebIDL bindings to using something CallArgs-like. Review of attachment 755973 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, but would be good to take another look once the comments below are addressed. ::: js/src/ion/CodeGenerator.cpp @@ +1416,5 @@ > Register obj = masm.extractObject(Address(StackPointer, 0), argObj); > JS_ASSERT(obj == argObj); > > // Push a Value containing the callee object: natives are allowed to access their callee before > // setitng the return value. The StackPointer is moved to &vp[0]. Nit: remove the last part: "The StackPointer..." since that movePtr is gone now. @@ +1417,5 @@ > JS_ASSERT(obj == argObj); > > // Push a Value containing the callee object: natives are allowed to access their callee before > // setitng the return value. The StackPointer is moved to &vp[0]. > masm.Push(ObjectValue(*target)); It seems like we can do masm.computeEffectiveAddress(Address(StackPointer, 2 * sizeof(Value)), argArgs); here to get vp + 2, if we no longer need argArgs for the argc below. @@ +1428,2 @@ > // Push argument into what will become the IonExitFrame > + masm.Push(argArgs); masm.Push(Imm32(call->numStackArgs()); We can get rid of the extra move now because we no longer have to pass argc as argument. @@ +1438,5 @@ > + JS_STATIC_ASSERT(JSJitMethodCallArgs::offsetOfArgc() == > + IonDOMMethodExitFrameLayout::offsetOfArgcFromArgv()); > + masm.computeEffectiveAddress(Address(StackPointer, > + sizeof(uintptr_t) + 2*sizeof(Value)), > + argArgs); And remove this call. ::: js/src/ion/shared/IonFrames-x86-shared.h @@ +433,5 @@ > IonExitFrameLayout exit_; > // This must be the last thing pushed, so as to stay common with > // IonDOMExitFrameLayout. > JSObject *thisObj_; > + Value* argv_; Nit: Value *argv_; (same for the ARM version).

> Nit: remove the last part: "The StackPointer..." since that movePtr is gone now. That's not a comment about the movePtr. That's a comment about where StackPointer points to after this Push() call, and I believe it's still correct, though I rephrased it a bit to make that clearer. Made the rest of the changes. Very good catches!

Comment on attachment 756308 [details] [diff] [review] Part 1 updated to Jan's review comments Review of attachment 756308 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Thanks for the reviews! https://hg.mozilla.org/integration/mozilla-inbound/rev/1907d472e5d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/840183b434d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/98ec2d7ed87f https://hg.mozilla.org/integration/mozilla-inbound/rev/20f763a69b1f https://hg.mozilla.org/integration/mozilla-inbound/rev/f047d17cdb78 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c733ec01b14

I pushed the bustage fix as https://hg.mozilla.org/integration/mozilla-inbound/rev/39a59be2a4cc for the moment...

And pushed that second bustage fix as http://hg.mozilla.org/integration/mozilla-inbound/rev/697190293f4e

https://hg.mozilla.org/mozilla-central/rev/1907d472e5d4 https://hg.mozilla.org/mozilla-central/rev/840183b434d2 https://hg.mozilla.org/mozilla-central/rev/98ec2d7ed87f https://hg.mozilla.org/mozilla-central/rev/20f763a69b1f https://hg.mozilla.org/mozilla-central/rev/f047d17cdb78 https://hg.mozilla.org/mozilla-central/rev/3c733ec01b14 https://hg.mozilla.org/mozilla-central/rev/39a59be2a4cc https://hg.mozilla.org/mozilla-central/rev/697190293f4e