Closed
Bug 724875
Opened 12 years ago
Closed 12 years ago
IonMonkey: Implement write barriers
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Assigned: dvander)
References
(Blocks 1 open bug)
Details
(Keywords: crash, testcase)
Attachments
(1 file, 3 obsolete files)
56.59 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
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); }
Assignee | ||
Updated•12 years ago
|
Blocks: IonMonkey
Summary: IonMonkey: Crash with SIGTRAP and gczeal(4) → IonMonkey: Implement write barriers
Assignee | ||
Updated•12 years ago
|
Assignee: general → dvander
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
Adds support for StoreElement[Hole]<T|V>
Attachment #597287 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #597667 -
Attachment is obsolete: true
Attachment #598112 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 598112 [details] [diff] [review] patch Marty for ARM changes.
Attachment #598112 -
Flags: review?(mrosenberg)
Comment 5•12 years ago
|
||
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.
Attachment #598112 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Thanks for the careful review!
Attachment #598112 -
Attachment is obsolete: true
Attachment #598112 -
Flags: review?(mrosenberg)
Attachment #598323 -
Flags: review?(mrosenberg)
Comment 7•12 years ago
|
||
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.
Attachment #598323 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/8add57bafb0d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 9•11 years ago
|
||
Automatically extracted testcase for this bug was committed: https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•