Closed Bug 579475 Opened 14 years ago Closed 14 years ago

JM: Make JSOP_SETELEM on dense arrays faster

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
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
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.
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.
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.
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.
Note that replacing arr[i] with arr[0] reduces JM time to 2 ms., so loop and call overhead is not that big here.
(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.
(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 :)
(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 1 isn't timing the hole-setting path AFAICT, because the loop outside of the timing region fills the array.
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.
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.
Can this now be resolved, per the implementation of bug 584917 mentioned in comment 12?
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.