We can use the array trim trick in old paged space as well as

We can use the array trim trick in old paged space as well as
new space.

Committed: http://code.google.com/p/v8/source/detail?r=5311

[Vitaly Repeshko, 2010-08-20 00:06:46]

Drive by observation.


-- Vitaly

http://codereview.chromium.org/3143032/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/1/2#newcode332
src/builtins.cc:332: return elms + to_trim * kPointerSize;
This line advances elms pointer by (to_trim * kPointerSize) fixed arrays, but
since our object classes have no data members, sizeof(FixedArray) happens to be
1 (the least allowed size), and advancing by this many fixed arrays is
equivalent to advancing by this many bytes. I think this is too subtle to rely
on.

[Erik Corry, 2010-08-20 07:57:59]

http://codereview.chromium.org/3143032/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/1/2#newcode332
src/builtins.cc:332: return elms + to_trim * kPointerSize;
On 2010/08/20 00:06:46, Vitaly wrote:
> This line advances elms pointer by (to_trim * kPointerSize) fixed arrays, but
> since our object classes have no data members, sizeof(FixedArray) happens to
be
> 1 (the least allowed size), and advancing by this many fixed arrays is
> equivalent to advancing by this many bytes. I think this is too subtle to rely
> on.

Good catch!

[Vyacheslav Egorov (Chromium), 2010-08-20 08:01:42]

LGTM

http://codereview.chromium.org/3143032/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/1/2#newcode332
src/builtins.cc:332: return elms + to_trim * kPointerSize;
There is an appropriate assertion just above return.

I do not understand why Anton decided to use such expression though.

On 2010/08/20 00:06:46, Vitaly wrote:
> This line advances elms pointer by (to_trim * kPointerSize) fixed arrays, but
> since our object classes have no data members, sizeof(FixedArray) happens to
be
> 1 (the least allowed size), and advancing by this many fixed arrays is
> equivalent to advancing by this many bytes. I think this is too subtle to rely
> on.

http://codereview.chromium.org/3143032/diff/1/2#newcode725
src/builtins.cc:725: const bool trim_array = Heap::new_space()->Contains(elms)
&&
You should also fix this condition.

[Erik Corry, 2010-08-20 10:04:21]

http://codereview.chromium.org/3143032/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/1/2#newcode725
src/builtins.cc:725: const bool trim_array = Heap::new_space()->Contains(elms)
&&
On 2010/08/20 08:01:42, Vyacheslav Egorov wrote:
> You should also fix this condition.

Thanks, I was wondering where that one was!

[antonm, 2010-08-20 11:25:46]

http://codereview.chromium.org/3143032/diff/10001/11001
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/10001/11001#newcode324
src/builtins.cc:324: }
forgotten zapping?

[antonm, 2010-08-20 11:50:30]

LGTM

http://codereview.chromium.org/3143032/diff/14001/2002
File src/builtins.cc (right):

http://codereview.chromium.org/3143032/diff/14001/2002#newcode328
src/builtins.cc:328: for (int i = 0; i < to_trim; i++) {
you may save one store here as elms->address() would be overwritten with filler
map in next store.  And actually more as you could skip the whole header, but it
might be too subtle to reason about.