1344691 - CacheIR: dense element stub should support element (re)allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

We use an IC stub for adding a dense element, but this stub checks the capacity and will fail if we need to (re)allocate elements. The attached patch fixes this to call a helper function to reallocate instead of failing the stub.

I need this for bug 1328140 to avoid regressing Talos CanvasMark, but it's nice to have anyway.

This also works when adding the first dense element. The micro-benchmark below improves from 3 seconds to 0.3 seconds and I see a similar change with --no-ion.

function f() {
    var t = new Date;
    for (var i=0; i<10000000; i++) {
        // Int32Array to force an IC in Ion.
        var a = (i == 100) ? new Int32Array(1) : {};
        a[0] = i;
    }
    print(new Date - t);
}
f();

Comment 1

[Tom S [:evilpie]]

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

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

Great, that is a huge perf difference. I am pretty sure I have seen cases where adding the first element to some object would help. Maybe just add this benchmark as a test to jit-test/cacheir?

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1122,3 @@
>          Address capacity(scratch, ObjectElements::offsetOfCapacity());
> +        masm.branch32(Assembler::Above, capacity, index, &capacityOk);
> +

I think we can just check ObjectElements::NONWRITABLE_ARRAY_LENGTH here, instead of doing this in the function.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80e4bee4db1
Make add-dense-element stub support element (re)allocation. r=evilpie

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #1)
> Maybe just add this benchmark as a test to jit-test/cacheir?

Done.

> I think we can just check ObjectElements::NONWRITABLE_ARRAY_LENGTH here,
> instead of doing this in the function.

Good point, that should be a little faster.

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d80e4bee4db1