As mentioned in bug 988950 comment 25: - ICStubCompiler::emitPostWriteBarrierSlot() can make use of MacroAssembler::branchValueIsNurseryObject() if the latter is extended to allow branching if the test fails. - The post barriers in BaselineCompiler.cpp don't always check that the value is a nursery object.

Here's a patch to do this. We need to add a condition argument to both nursery check instructions for this which complicates things unfortunately. I made BaselineCompiler::storeValue() return the register the value ended up in if any so we could check it in emitFormalArgAccess(), however this seems like overkill to me. Let me know what you think.

Comment on attachment 8412506 [details] [diff] [review] bug1000100-postbarrier-tidyup Review of attachment 8412506 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! Some comments below to simplify the code a bit, let me know what you think. ::: js/src/jit/BaselineCompiler.cpp @@ +2115,4 @@ > #ifdef JSGC_GENERATIONAL > // Fully sync the stack if post-barrier is needed. > // Scope coordinate object is already in R2.scratchReg(). > frame.syncStack(0); Please remove this while you're here, the popRegsAndSync(1) call above will make sure only R0 is live, so we can still grab R1.scratchReg(). Then we only have to make sure R0 is saved when we call the post-barrier. It'd be nice if we could push/pop it around the call in emitOutOfLinePostBarrierSlot, then add a comment to the call below like this: masm.call(&postBarrierSlot_); // Won't clobber R0. And add regs.take(R0) to emitOutOfLinePostBarrierSlot. SETALIASEDVAR is used like this most of the time: "x = .."; if we don't have to store the RHS to the stack we eliminate some instructions and the JSOP_POP that follows becomes a no-op. @@ +2119,5 @@ > Register temp = R1.scratchReg(); > > Label skipBarrier; > masm.branchTestObject(Assembler::NotEqual, R0, &skipBarrier); > + masm.branchPtrInNurseryRange(Assembler::Equal, objReg, temp, &skipBarrier); Change the branchTestObject to branchValueIsNurseryObject, and maybe swap these two calls, depending on what is most likely. @@ +2426,5 @@ > frame.push(R0); > } else { > masm.patchableCallPreBarrier(argAddr, MIRType_Value); > + StackValue *source = frame.peek(-1); > + TypedOrValueRegister val = storeValue(source, argAddr, R0); There's a syncStack(0) earlier so this should always hit the "kind == Stack" case. I think it's simpler to change it to: masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), R0); masm.storeValue(R0, argAddr); And use R0 below. @@ +2435,5 @@ > + JS_ASSERT(source->constant().isObject() || > + !nursery.isInside(&source->constant().toObject())); > + } else { > + // Fully sync the stack if post-barrier is needed. > + frame.syncStack(0); Stack should still be synced, so this can be MOZ_ASSERT(frame.numUnsyncedSlots() == 0);

Thanks for the comments. Here's the updated patch.

Comment on attachment 8414492 [details] [diff] [review] bug1000100-postbarrier-tidyup v2 Review of attachment 8414492 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r=me. ::: js/src/jit/BaselineCompiler.cpp @@ +2104,2 @@ > // Scope coordinate object is already in R2.scratchReg(). > frame.syncStack(0); Remove this call, the post barrier will save R0 now so this should work.