Correct large packed array length limitation

Correct large packed array length limitation

The length of packed array could be up to kInitialMaxFastElementArray instead of (kInitialMaxFastElementArray -1)

BUG=

[Jakob Kummerow, 2013-09-23 16:53:29]

I don't see what problem this CL is solving. AFAICS, kInitialMaxFastElementArray
is consistently used as an exclusive upper bound; this change introduces
inconsistency wrt. the use in ArrayConstructorCommon in runtime.cc.

What this patch essentially achieves is to bump the limit from 100,000 to
100,001. I don't see a reason to do that.

If I'm not mistaken, the recently added ASSERT in heap.cc is a little too
strict, we could relax it to use
FixedArray::SizeFor(JSObject::kInitialMaxFastElementArray - 1).

https://codereview.chromium.org/23441080/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/23441080/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:664: HInstruction* checked_arg =
Add<HBoundsCheck>(argument, max_alloc_length + 1);
No way!

[weiliang.lin2, 2013-09-24 02:22:17]

On 2013/09/23 16:53:29, Jakob wrote:
> I don't see what problem this CL is solving. AFAICS,
kInitialMaxFastElementArray
> is consistently used as an exclusive upper bound; this change introduces
> inconsistency wrt. the use in ArrayConstructorCommon in runtime.cc.
> 
> What this patch essentially achieves is to bump the limit from 100,000 to
> 100,001. I don't see a reason to do that.
> 
> If I'm not mistaken, the recently added ASSERT in heap.cc is a little too
> strict, we could relax it to use
> FixedArray::SizeFor(JSObject::kInitialMaxFastElementArray - 1).
> 
> https://codereview.chromium.org/23441080/diff/1/src/code-stubs-hydrogen.cc
> File src/code-stubs-hydrogen.cc (right):
> 
>
https://codereview.chromium.org/23441080/diff/1/src/code-stubs-hydrogen.cc#ne...
> src/code-stubs-hydrogen.cc:664: HInstruction* checked_arg =
> Add<HBoundsCheck>(argument, max_alloc_length + 1);
> No way!

Thanks for comments. 
Why I submitted this change is misc-basic-array micro in
http://www.arewefastyet.com/. I paste it below. It creates a large array with
100000 size. V8 suffers from the size limitation (Actually at first glance, V8
should work well in this micro, because in my memory, V8's max fast element
number could be 100000 because of the name of "kInitialMaxFastElementArray" ).  



function fill_array(len) {
    var a = Array(len);
    for (var i = 0; i < 500; i++) {
        for (var j = 0; j < len; j++)
            a[j] = 0;
    }
    return a;
}

function use_array(a) {
    for (var i = 0; i < 500; i++) {
        var x = 0;
        for (var j = 0; j < a.length; j++)
            x += a[j];
    }
}

var a = fill_array(100000);
use_array(a);



I overlooked the use in ArrayConstructorCommon in runtime.cc. (I think useing
"len" instead of "value" there makes it more meaningful.)

[Jakob Kummerow, 2013-09-24 08:31:32]

On 2013/09/24 02:22:17, weiliang.lin2 wrote:
> Why I submitted this change is misc-basic-array micro in
> http://www.arewefastyet.com/.

That's the trouble with microbenchmarks: they tend to make VM implementers waste
time on useless changes. I realize that since that benchmark misses our
heuristic by 1 it's tempting to just bump up the limit; but doing so does not
make V8 a better engine, so I think a better approach is to disregard the
benchmark -- it is meaningless. Let's focus our efforts on things that matter.

Also, for the record, the unintended pointer arithmetic in
code-stubs-hydrogen.cc is still a pretty bad bug.