Faster moving FixedArray elements around.

Faster moving FixedArray elements around.

[antonm, 2010-02-19 18:04:59]

Mads, Søren,

may you have a look?

Checking absence of this element incurs slight perf penalty (~3%), but I think
we'd better start with simpler approach and elaborate on it when we have better
infrastructure.

Inlining Heap::AllocateFixedArray apparently brought minimal benefit (I guess
due to small number of reallocations vs. moves thanks to our growth policy).

I've added two mostly identical tests verifying that everything works fine.  I
wanted to add __defineGetters__ variants, but getter didn't work for me for
elements.

Is there a way to get rid of duplication for those tests?

Dromaeo runs: http://dromaeo.com/?id=93461,93464

[Søren Thygesen Gjesse, 2010-02-24 21:48:59]

Sorry for the late review.

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

http://codereview.chromium.org/650043/diff/1/3#newcode390
src/builtins.cc:390: static bool ArrayPrototypeHasNoElements() {
Are these checks enough? I think you can introduce elements higher up the chain
like this:

  Array.prototype.__proto__ = {a:1}
  Array.prototype.__proto__.__proto__ = {"1":2}
  a = new Array()

a[1] is now 2. Are there any constraints that the array operations will only
look one step up the chain, or am I missing something here?
Do we have a test on the non-writability of Object and Array prototype fields?

http://codereview.chromium.org/650043/diff/1/3#newcode464
src/builtins.cc:464: // TODO(antonm): try to shift/copy RSet bits when
moving/copying.
Please open an issue and change this TODO to TODO(issue).

http://codereview.chromium.org/650043/diff/1/5
File src/objects.h (right):

http://codereview.chromium.org/650043/diff/1/5#newcode1629
src/objects.h:1629: // Gives access to raw memory which stores array's data.
array's -> the array's

http://codereview.chromium.org/650043/diff/1/8
File test/mjsunit/array-elements-from-array-prototype.js (right):

http://codereview.chromium.org/650043/diff/1/8#newcode31
test/mjsunit/array-elements-from-array-prototype.js:31: // If add any new tests
here, consider adding them to all four files:
four == two?

http://codereview.chromium.org/650043/diff/1/8#newcode174
test/mjsunit/array-elements-from-array-prototype.js:174: // now owned undefined
resides at 4 and rest is shifted right by one
Start comment uppercase and end with period.

http://codereview.chromium.org/650043/diff/1/9
File test/mjsunit/array-elements-from-object-prototype.js (right):

http://codereview.chromium.org/650043/diff/1/9#newcode29
test/mjsunit/array-elements-from-object-prototype.js:29: // Tests below verify
that elements set on Array.prototype propagate
Array -> Object?

http://codereview.chromium.org/650043/diff/1/9#newcode31
test/mjsunit/array-elements-from-object-prototype.js:31: // If add any new tests
here, consider adding them to all four files:
four == two?

http://codereview.chromium.org/650043/diff/1/9#newcode174
test/mjsunit/array-elements-from-object-prototype.js:174: // now owned undefined
resides at 4 and rest is shifted right by one
Start comment uppercase end with period.

http://codereview.chromium.org/650043/diff/1/10
File test/mjsunit/fuzz-natives.js (right):

http://codereview.chromium.org/650043/diff/1/10#newcode152
test/mjsunit/fuzz-natives.js:152: // That must only be invoked on
Array.prototype.
That must -> This can

[antonm, 2010-02-25 15:39:38]

Søren,

thanks a lot for first round of comments.

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

http://codereview.chromium.org/650043/diff/1/3#newcode390
src/builtins.cc:390: static bool ArrayPrototypeHasNoElements() {
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> Are these checks enough? I think you can introduce elements higher up the
chain
> like this:
> 
>   Array.prototype.__proto__ = {a:1}
>   Array.prototype.__proto__.__proto__ = {"1":2}
>   a = new Array()
> 
> a[1] is now 2. Are there any constraints that the array operations will only
> look one step up the chain, or am I missing something here?

I hope the checks are enough (I added another test though).  The
idea---ArrayPrototypeHasNoElements() checks the whole prototype chain (see
IsNull check at the very end).  One catch: if one would manage to setup an
object with indexed interceptor getter, that might be a problem.

Alas, new check makes it slower somewhat.  I have an idea how to speed up this
check, but even this slower variant is still worth committing.

> Do we have a test on the non-writability of Object and Array prototype fields?

I didn't find immediately, but added one.  Is there a better place?  I'll add
another for Object and probably other types as well if overall approach is fine.

http://codereview.chromium.org/650043/diff/1/3#newcode464
src/builtins.cc:464: // TODO(antonm): try to shift/copy RSet bits when
moving/copying.
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> Please open an issue and change this TODO to TODO(issue).

Just removed.  I'm working on it, so won't forget about it.

http://codereview.chromium.org/650043/diff/1/5
File src/objects.h (right):

http://codereview.chromium.org/650043/diff/1/5#newcode1629
src/objects.h:1629: // Gives access to raw memory which stores array's data.
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> array's -> the array's

Done.

http://codereview.chromium.org/650043/diff/1/8
File test/mjsunit/array-elements-from-array-prototype.js (right):

http://codereview.chromium.org/650043/diff/1/8#newcode31
test/mjsunit/array-elements-from-array-prototype.js:31: // If add any new tests
here, consider adding them to all four files:
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> four == two?

Ah, yes, I planned to have four (for getters), but ended with two.  Now with
three.  Thanks a lot for spotting this.

http://codereview.chromium.org/650043/diff/1/8#newcode174
test/mjsunit/array-elements-from-array-prototype.js:174: // now owned undefined
resides at 4 and rest is shifted right by one
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> Start comment uppercase and end with period.

Done.

http://codereview.chromium.org/650043/diff/1/9
File test/mjsunit/array-elements-from-object-prototype.js (right):

http://codereview.chromium.org/650043/diff/1/9#newcode29
test/mjsunit/array-elements-from-object-prototype.js:29: // Tests below verify
that elements set on Array.prototype propagate
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> Array -> Object?

Done.

http://codereview.chromium.org/650043/diff/1/9#newcode31
test/mjsunit/array-elements-from-object-prototype.js:31: // If add any new tests
here, consider adding them to all four files:
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> four == two?

Done.

http://codereview.chromium.org/650043/diff/1/9#newcode174
test/mjsunit/array-elements-from-object-prototype.js:174: // now owned undefined
resides at 4 and rest is shifted right by one
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> Start comment uppercase end with period.

Done.

http://codereview.chromium.org/650043/diff/1/10
File test/mjsunit/fuzz-natives.js (right):

http://codereview.chromium.org/650043/diff/1/10#newcode152
test/mjsunit/fuzz-natives.js:152: // That must only be invoked on
Array.prototype.
On 2010/02/24 21:48:59, Søren Gjesse wrote:
> That must -> This can

Done.