1341071 - Fix Ion IC post write barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

MPostWriteBarrier instructions are added in IonBuilder when we add a SetProp/SetElem IC.

This works but is a bit weird because in a lot of cases (setters, proxies, the store happening in the VM, etc) it will add a barrier and fill the store buffer for no good reason. Furthermore, we will optimize more things in the future and we might do the store on an object that's different from the object we have in IonBuilder.

To fix these issues the IC stubs should have their own post barriers (like the Baseline stubs).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Instead of adding a post barrier in IonBuilder for the SetProp/SetElem IC, this adds the barrier to the IC stubs we attach, like Baseline.

This avoids adding unnecessary barriers (and slowing down nursery GCs) when all we're doing is pass a nursery object to a setter or proxy trap, or when we don't attach IC stubs but do the store in the VM.

The patch also changes the IC code to use the per-element barrier optimization (see PostWriteElementBarrier) when possible. Before, we only did that in Ion and only for int32 indexes. The micro-benchmark below improves from 3200 ms to 739 ms because of this.

We should add fewer objects to the whole cell buffer than before, so this should have a positive effect on our nursery GC times.

function f() {
    var a = [];
    var t = new Date;
    for (var i=0; i<10000000; i++) {
        a["" + i] = {};
        if (i > 0)
            a[i-1] = null;
    }
    print(new Date - t);
}
f();

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8844041 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +11675,1 @@
>              bool needsBarrier = objTypes->propertyNeedsBarrier(constraints(), NameToId(name));

I guess |needsBarrier| is a different barrier - looking at the code it's for the pre barrier.  Can we rename this to |needsPreBarrier|?  Also below.

::: js/src/jit/VMFunctions.cpp
@@ +665,5 @@
>  }
>  
>  static const size_t MAX_WHOLE_CELL_BUFFER_SIZE = 4096;
>  
> +template <bool IndexInBounds>

Could you make this an enum to make it clearer at the callsite what's happening?

@@ +672,5 @@
>  {
>      MOZ_ASSERT(!IsInsideNursery(obj));
> +
> +    if (IndexInBounds) {
> +        MOZ_ASSERT(uint32_t(index) < obj->as<NativeObject>().getDenseInitializedLength());

It's not clear to me how we know that obj is always a native object at this point, but I expect that's handled by the JIT.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8844041 [details] [diff] [review]
Patch

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

Thanks, this is indeed cleaner to have the post-write-barrier next to the store at the assembly level, instead of being at the IonBuilder level.

::: js/src/jit/CodeGenerator.cpp
@@ +3855,5 @@
>      masm.mov(ImmPtr(GetJitContext()->runtime), runtimereg);
>      masm.passABIArg(runtimereg);
>      masm.passABIArg(objreg);
>      masm.passABIArg(indexreg);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, (PostWriteElementBarrier<false>)));

nit: Add a AutoAssertNoGC in PostWriteElementBarrier function.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51bffdbb6a30
Fix Ion IC post barriers to be part of the IC stubs instead of adding them in IonBuilder. r=jonco,nbp

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51bffdbb6a30