774502 - IonMonkey: Remove extra SetInitializedLength from jsop_initelem_dense if not followed by GC.

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The current order of the bytecode for inlined array declaration implies that we are allocating the array before the computation of each element and each time a new element is computed, we add it and set the initializedLength of the array.

We can either re-order the bytecode such as the initialization of the array is done once all elements are computed, but this might cause more stack allocation for big arrays or we might consider removing the extra SetInitializedLength when they are not followed by any operations which can cause a GC — except for the last one.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Remove extra SetInitializedLength.

This quick implementation which does not handle cross basic blocks folding of SetInitializedLength can do a 6% better on the assorted test bugs-608733-trace-compiler (1855.2ms -> 1745.4ms).

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] from comment #1)
> Created attachment 642793 [details] [diff] [review]
> Remove extra SetInitializedLength.
> 
> This quick implementation which does not handle cross basic blocks folding
> of SetInitializedLength can do a 6% better on the assorted test
> bugs-608733-trace-compiler (1855.2ms -> 1745.4ms).

Ok, I'll reclaim these 6% and put them on some random compiler behaviour because jsop_initelem_dense is never called by the only function compiled by IonMonkey.

I recompiled the base and the patched version and I cannot explain this 6% for now.

Comment 3

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

Comment on attachment 642793 [details] [diff] [review]
Remove extra SetInitializedLength.

GC instructions don't necessarily mark as effectful - see, for example, MConcat.

Comment 4

[Nicolas B. Pierron [:nbp]]

On the following benchmark, this optimization is worth 7.5%, and likely increasing with the size of the array.  With 3 elements (common in benchmarks) there is almost no differences.  The patch is around 1-2% on kraken.

function f (a,b,c,d,e,f,g,h,i) {
    return [a,b,c,d,e,f,g,h,i];
}

function g() {
    for (var i = 0; i < 100000000; i++)
        f(1,2,3,4,5,6,7,8,9);
}

g();


(In reply to David Anderson [:dvander] from comment #3)
> GC instructions don't necessarily mark as effectful - see, for example,
> MConcat.

Indeed. The goal of this patch was to test the performance impact of a similar modification which should account for GCs, so the effectful flag is just a shortcut in this case.  Next time I will just flag it as feedback instead of review.

Unassigned for now.

Comment 5

[Nicolas B. Pierron [:nbp]]

Doing this optimization would be trivial on top of Scalar Replacement feature (Bug 897606).

As we might want to use alias analysis to detect that an expression can alias this array (a call), and read its initialized length, as well as setting this property inside the side-effects list of resume points which are present.

The patch that I did before checks that we have no additional resume points since the previous instruction, which is correct, but not as efficient as using the scalar replacement approach.