Open
Bug 774502
Opened 12 years ago
Updated 2 years ago
IonMonkey: Remove extra SetInitializedLength from jsop_initelem_dense if not followed by GC.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
NEW
People
(Reporter: nbp, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ion:t])
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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).
Attachment #642793 -
Flags: review?(dvander)
Reporter | ||
Comment 2•12 years ago
|
||
(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 on attachment 642793 [details] [diff] [review]
Remove extra SetInitializedLength.
GC instructions don't necessarily mark as effectful - see, for example, MConcat.
Attachment #642793 -
Flags: review?(dvander)
Reporter | ||
Comment 4•12 years ago
|
||
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.
Assignee: nicolas.b.pierron → general
Status: ASSIGNED → NEW
Updated•12 years ago
|
Whiteboard: [ion:t]
Reporter | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Assignee: general → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•