Closed
Bug 579475
Opened 14 years ago
Closed 14 years ago
JM: Make JSOP_SETELEM on dense arrays faster
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 584917
People
(Reporter: dmandelin, Unassigned)
References
Details
Make this fast:
function f() {
var a = new Array(100);
for (var i = 0; i < 100; ++i)
a[i] = false;
var t0 = new Date;
for (var i = 0; i < 27000; ++i) {
for (var j = 0; j < 100; ++j)
a[j] = false;
}
print(new Date - t0);
}
f();
SunSpider does about 2.7M dense array sets.
JM wanted SS win: 5 ms
Comment 1•14 years ago
|
||
Where are you getting 5ms from for this one? I get these times:
JM: 13ms
JSC: 12ms
V8: 9ms
Commenting out the 'a[j] = false', I get these times:
JM: 7ms
JSC: 6ms
V8: 4ms
It looks like most of the differences are due to general loop stuff (loop variable register allocation, interrupt checks) rather than to differences in setelem.
Reporter | ||
Comment 2•14 years ago
|
||
On buildmonkey-right, I get:
empty-loop version original ubench
JM 8 16
JSC 7 12
I'm not too surprised you don't see a difference, though. Perf differences for JSC vs. JM on filling arrays seem to depend a lot on the size of the array, so there is probably something funny going on.
Reporter | ||
Comment 3•14 years ago
|
||
By the way, I came up with this bug mostly from profiling crypto-aes and seeing that we spend 13% of our time in setelem. Maybe that is not really a slowdown for us, though. You might want to check on that benchmark and see if you can find the slowdowns elsewhere.
Comment 4•14 years ago
|
||
This also slows down access-nsieve. Reduced testcase:
-----------
function test(arr) {
for(var i=0; i<40000; i++)
arr[i] = true;
}
var t0 = new Date;
for(var x=0; x<5; x++) {
var arr = Array(40000);
test(arr);
}
print((new Date)-t0);
-----------
On my P4 I get these times:
- JM: 13 ms.
- TM: 9 ms.
- V8: 4 ms.
Comment 5•14 years ago
|
||
Note that replacing arr[i] with arr[0] reduces JM time to 2 ms., so loop and call overhead is not that big here.
Comment 6•14 years ago
|
||
(In reply to comment #5)
> Note that replacing arr[i] with arr[0] reduces JM time to 2 ms., so loop and
> call overhead is not that big here.
This is stressing setelem on holes, see bug 580355.
Comment 7•14 years ago
|
||
(In reply to comment #6)
> (In reply to comment #5)
> > Note that replacing arr[i] with arr[0] reduces JM time to 2 ms., so loop and
> > call overhead is not that big here.
>
> This is stressing setelem on holes, see bug 580355.
Sorry. Great results there btw :)
Is this fixed by bug 580355?
Comment 9•14 years ago
|
||
(In reply to comment #8)
> Is this fixed by bug 580355?
No difference between tip and revision aabc31b1b29e (right before bug 580355 landed) for the test case in comment 1. The code in comment 4 got much faster, as Brian Hackett predicted.
Comment 10•14 years ago
|
||
Comment 1 isn't timing the hole-setting path AFAICT, because the loop outside of the timing region fills the array.
Reporter | ||
Comment 11•14 years ago
|
||
This seems like it's still biting us. We are *much* slower than V8 on this:
function f() {
for (var i = 0; i < 1000000; ++i) {
var a = new Array(4);
a[0] = 33;
}
}
var t0 = new Date;
f();
print(new Date - t0);
We are 60 ms slower (on a fast Windows machine) on the 'new Array' part. That's not such a big deal: in SunSpider there are about 27000 'new Array', so we are losing only 1-2 ms here. But that's about a 5x slowdown, and it may bite more on other workloads, so we should fix that too.
We are 196 ms slower on the 'a[0] = 33' part, or about 60x slower (199 ms vs 3 ms for that part). If I add 3 more sets, we get only 26 ms slower than that, or about 4x (35 ms vs. 9 ms). So, something is very bad the first time we set a value on an array.
Comment 12•14 years ago
|
||
This will be fixed by bug 584917. The assign to a[0] currently mallocs an array, and it won't with 584917 (slots allocated inline). 584917 is ready to go; it will probably conflict somewhat with the compartments stuff blocking beta 7 so I'm waiting for that to land first.
Comment 13•14 years ago
|
||
Can this now be resolved, per the implementation of bug 584917 mentioned in comment 12?
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•