TraversableMap only dependent on DescriptorArray

TraversableMap only dependent on DescriptorArray
This is the first step towards merging the ContentArray into the DescriptorArray, (hence reclaiming 3 words per descriptor array).

Committed: https://code.google.com/p/v8/source/detail?r=11665

[Michael Starzinger, 2012-05-24 14:02:25]

This is going into the right direction. Most of my comments are just nits.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1947: 
Two empty lines between top-level methods.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1955: Object* details =
RawGetContentArray()->get(ToDetailsIndex(descriptor_number));
More than 80 characters.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4901: // Attention, tricky index manipulation ahead: Two two
consecutive indices
Only one "two".

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4907: while ((index / 2) <
descriptor_array_->number_of_descriptors()) {
Can we hoist the division by two into a local variable? That would improve
readability. Like this:

int index = Smi::cast(*DescriptorArrayHeader())->value();
int descriptor_number = raw_index / 2;

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4919:
static_cast<AccessorPair*>(descriptor_array_->RawGetValue(index / 2));
More than 80 characters.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:5082: IntrusiveMapTransitionIterator
descriptor_iterator(MutatedInstanceDescriptors());
Could we just pass the map itself to the constructor of the child iterator and
do the raw access of the instance descriptor field in there?

More than 80 characters.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h#newcode2479
src/objects.h:2479: inline Object* RawGetValue(int descriptor_number);
These methods are only needed by the IntrusiveMapTransitionIterator, can we move
them into that class (into the private section)? That prevents others from
accidentally using these methods.

Also I would rename them to GetRawValue and GetEawDetails which is more natural
sounding.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h#newcode2657
src/objects.h:2657: inline FixedArray* RawGetContentArray();
Likewise.

[Toon Verwaest, 2012-05-25 11:36:46]

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1947: 
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> Two empty lines between top-level methods.

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1955: Object* details =
RawGetContentArray()->get(ToDetailsIndex(descriptor_number));
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> More than 80 characters.

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4901: // Attention, tricky index manipulation ahead: Two two
consecutive indices
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> Only one "two".

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4907: while ((index / 2) <
descriptor_array_->number_of_descriptors()) {
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> Can we hoist the division by two into a local variable? That would improve
> readability. Like this:
> 
> int index = Smi::cast(*DescriptorArrayHeader())->value();
> int descriptor_number = raw_index / 2;

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:4919:
static_cast<AccessorPair*>(descriptor_array_->RawGetValue(index / 2));
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> More than 80 characters.

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.cc#newcode...
src/objects.cc:5082: IntrusiveMapTransitionIterator
descriptor_iterator(MutatedInstanceDescriptors());
As discussed offline, leaving it as it is.

On 2012/05/24 14:02:25, Michael Starzinger wrote:
> Could we just pass the map itself to the constructor of the child iterator and
> do the raw access of the instance descriptor field in there?
> 
> More than 80 characters.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h#newcode2479
src/objects.h:2479: inline Object* RawGetValue(int descriptor_number);
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> These methods are only needed by the IntrusiveMapTransitionIterator, can we
move
> them into that class (into the private section)? That prevents others from
> accidentally using these methods.
> 
> Also I would rename them to GetRawValue and GetEawDetails which is more
natural
> sounding.

Done.

https://chromiumcodereview.appspot.com/10411067/diff/1/src/objects.h#newcode2657
src/objects.h:2657: inline FixedArray* RawGetContentArray();
On 2012/05/24 14:02:25, Michael Starzinger wrote:
> Likewise.

Done.

[Toon Verwaest, 2012-05-25 11:54:41]

Feedback addressed, please take another look.

[Michael Starzinger, 2012-05-25 12:18:00]

Almost there, just three more nits.

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.cc#newc...
src/objects.cc:4922: ((raw_index & 1) == 0) ?
accessors->getter():accessors->setter();
Spaces to the left and right of ":". I know it will break the line, but we just
don't compress operators to avoid line breaking.

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h#newco...
src/objects.h:2423: friend class IntrusiveMapTransitionIterator;
Move that to the bottom of the class (or at least into the private section).

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h#newco...
src/objects.h:2480: 
Remove this empty line.

[Toon Verwaest, 2012-05-25 12:32:09]

Comments addressed, please take another look.

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.cc#newc...
src/objects.cc:4922: ((raw_index & 1) == 0) ?
accessors->getter():accessors->setter();
On 2012/05/25 12:18:00, Michael Starzinger wrote:
> Spaces to the left and right of ":". I know it will break the line, but we
just
> don't compress operators to avoid line breaking.

Done.

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h#newco...
src/objects.h:2423: friend class IntrusiveMapTransitionIterator;
On 2012/05/25 12:18:00, Michael Starzinger wrote:
> Move that to the bottom of the class (or at least into the private section).

Done.

https://chromiumcodereview.appspot.com/10411067/diff/7001/src/objects.h#newco...
src/objects.h:2480: 
On 2012/05/25 12:18:00, Michael Starzinger wrote:
> Remove this empty line.

Done.

[Michael Starzinger, 2012-05-25 12:39:43]

LGTM.