724875 - IonMonkey: Implement write barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on ionmonkey revision c34398f961e7 (run with --ion -n -m --ion-eager), tested on 64 bit:


var lfcode = new Array();
lfcode.push("gczeal(4);");
lfcode.push("");
while (true) {
	var file = lfcode.shift(); 
	if (file == undefined) { break; }
        loadFile(file);
}
function loadFile(lfVarx) {
	eval(lfVarx);
}

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v1

Adds write barriers for StoreSlotT/V. This approach places the barrier in the inline path, though most of the barrier is in a shared trampoline. The inline path has:

   if-gcthing <vp-address>:
     push <reg>
     lea <vp-address>, <reg>
     call <barrier>
     pop <reg>

The barrier function saves/restores volatile regs before calling a C++ function. That's a lot of registers to save though so it will be pretty slow. If that's a problem I'll try to inline more of the barrier. Another option would be capturing safepoints and saving registers in the inline path instead, but I'd like to avoid this.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v2

Adds support for StoreElement[Hole]<T|V>

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Turning write barriers always-on, I couldn't see a perf difference in ss/v8. That's not a huge surprise since we still have a lot to do. So I wrote a small microbenchmark that filled an array with objects, then overwrote every slot in the array:

    var t = [];
    var q = [];
    for (var i = 0; i < 10000000; i++) {
        t[i] = q;
    // Loop that gets timed
    for (var i = 0; i < 10000000; i++)
        t[i] = q;

-m -n without barriers got 18ms, and -m -n with barriers got 80ms.
--ion -n without barriers got 10ms, and --ion -n with barriers got 55ms.

So, it looks like both take a pretty bad worst case hit, but since this only happens during igc, I'm not worried. And we can work on inlining it later since the barrier code is isolated to one function.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 598112 [details] [diff] [review]
patch

Marty for ARM changes.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 598112 [details] [diff] [review]
patch

Review of attachment 598112 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I like how this requires only two lines of code for most instructions.

::: js/src/ion/Ion.cpp
@@ +1148,5 @@
>  
> +void
> +ion::MarkFromIon(JSCompartment *comp, Value *vp)
> +{
> +    //gc::MarkValueUnbarriered(comp->barrierTracer(), *vp, "write barrier");

This will be uncommented when incremental GC lands?

::: js/src/ion/IonMacroAssembler.h
@@ +103,5 @@
>          return size();
>      }
>  
> +    bool oom() const {
> +        return enoughMemory_ && MacroAssemblerSpecific::oom();

This should be "return !enoughMemory_ || MacroAssemblerSpecific::oom();"

@@ +283,5 @@
>              branch32(cond, dest, Imm32(key.constant()), label);
>      }
> +
> +    template <typename T>
> +    void emitPreBarrier(const T &address, JSValueType type) {

Nit: if this used MIRType instead of JSValueType, we could avoid some MIRType to JSValueType conversions. However if you think JSValueType is more appropriate here that's fine with me.

::: js/src/ion/MIR.h
@@ +3158,5 @@
>  class MSetPropertyInstruction : public MBinaryInstruction
>  {
>      JSAtom *atom_;
>      bool strict_;
> +    bool needsBarrier_;

Initialize this field in the constructor.

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +242,5 @@
>      const LAllocation *value = store->value();
>      MIRType valueType = store->mir()->value()->type();
>  
> +    if (store->mir()->needsBarrier())
> +        masm.emitPreBarrier(Address(base, offset), ValueTypeFromMIRType(store->mir()->slotType()));

I ignored most ARM changes, but CodeGeneratorARM::visitStoreSlotT needs this, too.

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +607,5 @@
>      }
> +
> +    template <typename T>
> +    void computeEffectiveAddress(const T &address, Register reg) {
> +        lea(Operand(address), PreBarrierReg);

I was going to say s/PreBarrierReg/reg, but a similar function was added to MacroAssembler-x86-shared so this one can be removed.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Thanks for the careful review!

Comment 7

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 598323 [details] [diff] [review]
v2

Review of attachment 598323 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +688,5 @@
> +
> +    RegisterSet save(GeneralRegisterSet(Registers::VolatileMask),
> +                     FloatRegisterSet(FloatRegisters::VolatileMask));
> +    masm.PushRegsInMask(save);
> +

If we want to be really pedantic about it, r1 will be saved here, but it doesn't need to be, since it is saved in the wrapper.

@@ +692,5 @@
> +
> +    JS_ASSERT(PreBarrierReg == r1);
> +    masm.movePtr(ImmWord(cx->compartment), r0);
> +
> +    masm.setupUnalignedABICall(2, r2);

I'd be happier if this were setupAlignedABICall, but this will work.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/projects/ionmonkey/rev/8add57bafb0d

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397