701990 - IonMonkey: Merge write barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Write barriers have landed on mozilla-central (yay), so the next merge into ionmonkey will have to add support for these. After talking with Bill today, I think these are roughly the steps needed.

Overview: The incremental GC is snapshot-at-the-beginning. During marking, if a reference is overwritten (say X.Y = Z), it may need a write barrier: if Z is overwriting an unmarked object, X.Y must be added to the mark queue.

(1) IonMonkey's weak pointers need to have read barriers. That includes all the IonCode references in IonCompartment, as well as the VMFunction cache. This is because weak pointers are not roots, so if (X.Y = *WeakRef), and we don't trigger a write barrier, the WeakRef won't be traced and might be collected.

(2) bug 700303 introduces LStoreSlotV and LStoreSlotT. We need write barrier support for these. In the best case, we know the type in the slot and can avoid the barrier entirely. Bill also points out that currently in JM+TI, we discard all JIT code before and after IGC. This means whether or not barriers are needed at all is a compile-time constant, and the barriers themselves can be inlined.

We don't support on-stack invalidation yet, but in the interim we could have a quick critical-path test for whether the barrier is needed, and then place the rest in out-of-line code (from the looks of JM+TI, the OOL code is a few checks and then, worst case, a call out to MarkValueUnbarriered).

(3) MarkIonCode needs a MarkIonCodeUnbarriered version.

(4) IonScript references to IonCode objects must be wrapped in HeapPtr<>.

I think that's everything...

Comment 1

[Sean Stangl [:sstangl]]

Attachment: Post-merge IonMonkey changes for write barriers.

This patch represents write barriers explicitly in the LIR via LWriteBarrierV and LWriteBarrierT. Code generation for these instructions trips breakpoints instead of calling to a C++ stub, since Nicolas has advised to wait for the C++ calls patch, at which point calling out to a stub function is trivial.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry for the drive-by review, but I was interested in the patch.

I think in IonCompartment::mark, you probably want to change the MarkIonCodeUnbarriered calls to MarkRoot, since IonCompartment::mark will be called during root marking. MarkRoot is safer and less sketchy than MarkUnbarriered because we can assert that it's only called during root marking.

For all the other Mark...Unbarriered calls, I've tried to maintain the practice of putting in a comment explaining why it's safe not to barrier. In these cases, it seems like it's because the data is immutable. Is that right? Is there any chance that any of these pointers will be patched?

Comment 3

[Sean Stangl [:sstangl]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Sorry for the drive-by review, but I was interested in the patch.

Thanks! It's much appreciated.

> MarkRoot is safer and less sketchy than MarkUnbarriered because we can
> assert that it's only called during root marking.

Thanks; done.

> Is there any chance that any of these pointers will be patched?

Not currently. We rely on discarding IonCode around GC, which is probably not sustainable in the long-term.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 575355 [details] [diff] [review]
Post-merge IonMonkey changes for write barriers.

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

::: js/src/ion/IonCompartment.h
@@ +71,4 @@
>      IonActivation *active_;
>  
>      // Trampoline for entering JIT code.
> +    ReadBarriered<IonCode> enterJIT_;

Don't we want a write barrier here ?  This is not a weakref. Otherwise it should not appear in IonCompartment::mark function.

Comment 5

[Sean Stangl [:sstangl]]

(In reply to Nicolas B. Pierron [:pierron] from comment #4)
> Comment on attachment 575355 [details] [diff] [review] [diff] [details] [review]

> Don't we want a write barrier here ?  This is not a weakref. Otherwise it
> should not appear in IonCompartment::mark function.

I think a read barrier is sufficient: we know that the IonCode, if it exists, will not be overwritten, and IonCompartment::mark() will already set existing code as a root. If the code is written after marking, using it will trigger the read barrier, which will mark it (whereas if it had a write barrier, it would not be marked, unless we changed IonCode::writeBarrierPost() to do so). So using a read barrier should be safe.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 575355 [details] [diff] [review]
Post-merge IonMonkey changes for write barriers.

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

::: js/src/ion/IonBuilder.cpp
@@ +2158,5 @@
>  
> +#ifdef JSGC_INCREMENTAL
> +    // Determine whether write barrier is required.
> +    if (cx->compartment->needsBarrier() &&
> +        (!propertyTypes || propertyTypes->needsBarrier(cx))) {

Nit: bracket on newline per style guide.

::: js/src/ion/LIR-Common.h
@@ +761,5 @@
> +
> +    LWriteBarrierV()
> +    { }
> +
> +    static const size_t Input = 0;

Can we call this constant something more descriptive, like, InputOperand?

::: js/src/ion/Lowering.cpp
@@ +561,5 @@
> +LIRGenerator::emitWriteBarrier(MInstruction *ins, MDefinition *input)
> +{
> +#ifdef JSGC_INCREMENTAL
> +    JS_ASSERT(GetIonContext()->cx->compartment->needsBarrier());
> +    LInstruction *barrier;

Nit: can we push this decl into a MIRType_Value case block?

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +306,5 @@
> +
> +    Label skipBarrier;
> +    masm.branchTestGCThing(Assembler::NotEqual, value, &skipBarrier);
> +    {
> +        masm.breakpoint();

You need another breakpoint here, and below, for consistency! :-)

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +167,5 @@
>          cmpl(tag, Imm32(JSVAL_UPPER_INCL_TAG_OF_NUMBER_SET));
>          return cond == Equal ? BelowOrEqual : Above;
>      }
> +    Condition testGCThing(Condition cond, Register tag) {
> +        JS_ASSERT(cond == Equal || cond == NotEqual);

Shouldn't this assert be in all of them?

Comment 7

[Sean Stangl [:sstangl]]

(In reply to Chris Leary [:cdleary] from comment #6)
> Can we call this constant something more descriptive, like, InputOperand?

It's currently idiomatic to use this style -- we should replace it with an enum-based idiom in the future.

> > +    Condition testGCThing(Condition cond, Register tag) {
> > +        JS_ASSERT(cond == Equal || cond == NotEqual);
> 
> Shouldn't this assert be in all of them?

They all call this function, so we're in the clear :)

Comment 8

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/83dbfff7f193
http://hg.mozilla.org/projects/ionmonkey/rev/422443a2e859

Comment 9

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

We should be able to do the write barriers now, we don't even need safe points or VM calls. We just need an out-of-line call to C++. It's not fallible and we can't GC.