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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
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();
Attachment #8843934 - Flags: review?(evilpies)
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.
Attachment #8843934 - Flags: review?(evilpies) → review+

Comment 2

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80e4bee4db1
Make add-dense-element stub support element (re)allocation. r=evilpie
(Assignee)

Comment 3

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d80e4bee4db1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1345707
You need to log in before you can comment on or make changes to this bug.