Last Comment Bug 724875 - IonMonkey: Implement write barriers
: IonMonkey: Implement write barriers
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: IonMonkey langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-02-07 07:06 PST by Christian Holler (:decoder)
Modified: 2013-02-07 05:20 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (39.88 KB, patch)
2012-02-14 20:23 PST, David Anderson [:dvander]
no flags Details | Diff | Review
WIP v2 (44.67 KB, patch)
2012-02-15 19:56 PST, David Anderson [:dvander]
no flags Details | Diff | Review
patch (54.18 KB, patch)
2012-02-16 19:48 PST, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Review
v2 (56.59 KB, patch)
2012-02-17 12:05 PST, David Anderson [:dvander]
marty.rosenberg: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-02-07 07:06:54 PST
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] 2012-02-14 20:23:53 PST
Created attachment 597287 [details] [diff] [review]
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] 2012-02-15 19:56:09 PST
Created attachment 597667 [details] [diff] [review]
WIP v2

Adds support for StoreElement[Hole]<T|V>
Comment 3 David Anderson [:dvander] 2012-02-16 19:48:56 PST
Created attachment 598112 [details] [diff] [review]
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] 2012-02-16 19:49:22 PST
Comment on attachment 598112 [details] [diff] [review]
patch

Marty for ARM changes.
Comment 5 Jan de Mooij [:jandem] 2012-02-17 04:06:57 PST
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] 2012-02-17 12:05:57 PST
Created attachment 598323 [details] [diff] [review]
v2

Thanks for the careful review!
Comment 7 Marty Rosenberg [:mjrosenb] 2012-02-17 19:29:12 PST
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] 2012-02-21 14:47:42 PST
https://hg.mozilla.org/projects/ionmonkey/rev/8add57bafb0d
Comment 9 Christian Holler (:decoder) 2013-02-07 05:20:57 PST
Automatically extracted testcase for this bug was committed:

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

Note You need to log in before you can comment on or make changes to this bug.