Promoting elements transitions to their own field.

Promoting elements transitions to their own field.
This is a first step towards separating all transitions from the property descriptions. If we link the property descriptions from the transition object, this will in allow the descriptor array (property descriptions) to become immutable.

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

[Toon Verwaest, 2012-05-29 15:04:31]

Promoting elements transition is the first step to extracting all transitions
from the descriptor array.

Extracting all the transitions will allow us to link map -> transitions ->
descriptor array. In such a setup, the descriptor array becomes immutable; and
we can start looking at ways to share (parts of) the descriptor array rather
than always copying it. This change will eat up the space freed up by getting
rid of the ContentArray.

Currently non-fast-elements-kinds are never cached. We should look into
attaching dictionary at the end of the elements-kinds array.

[danno, 2012-06-01 13:49:55]

http://codereview.chromium.org/10444055/diff/4005/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/10444055/diff/4005/src/factory.cc#newcode918
src/factory.cc:918: if (LinearSearchUnsorted(*result, *key, descriptor_count) ==
Add a enum { EXPECT_SORTED, EXPECT_UNSORTED } and put both implementations in
LinearSearch on the descriptor that take the enum selector as another parameter.

http://codereview.chromium.org/10444055/diff/4005/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/10444055/diff/4005/src/mark-compact.cc#newcode...
src/mark-compact.cc:1867: // printf("marking: %p\n",
static_cast<void*>(descriptors));
Remove debugging code.

http://codereview.chromium.org/10444055/diff/4005/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/10444055/diff/4005/src/objects-inl.h#newcode1874
src/objects-inl.h:1874: // immutable again.
Re-enable this assert appropriately.

http://codereview.chromium.org/10444055/diff/4005/src/objects-printer.cc
File src/objects-printer.cc (right):

http://codereview.chromium.org/10444055/diff/4005/src/objects-printer.cc#newc...
src/objects-printer.cc:275: break;
Please add a printer for the transition map in the descriptor array so that it's
possible to output this information.

http://codereview.chromium.org/10444055/diff/4005/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/10444055/diff/4005/src/objects.cc#newcode1571
src/objects.cc:1571: {
nit: No need for this whitespace change.

http://codereview.chromium.org/10444055/diff/4005/src/objects.cc#newcode2321
src/objects.cc:2321: // Only remember the map transition if the object's map is
NOT equal to
nit: indentation

http://codereview.chromium.org/10444055/diff/4005/src/objects.cc#newcode4039
src/objects.cc:4039: { MaybeObject* maybe = map()->CopyDropTransitions(false);
This should be an enum rather than a bool, it's really difficult to see what the
parameter means at the usage site.

http://codereview.chromium.org/10444055/diff/4005/src/objects.cc#newcode4945
src/objects.cc:4945: if (descriptor_array_->MayContainTransitions())
This is confusing. What does descriptor iteration have to do with transition?
Perhaps another predicate name, even if it does the same thing?

http://codereview.chromium.org/10444055/diff/4005/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/10444055/diff/4005/src/objects.h#newcode2405
src/objects.h:2405: DECL_ACCESSORS(elements_transition, Map)
probably should call this elements_transition_map

http://codereview.chromium.org/10444055/diff/4005/src/objects.h#newcode2409
src/objects.h:2409: ASSERT(length() >= kFirstIndex || IsEmpty());
ASSERT(length() > kFirstIndex || length() == kTransitionsIndex || IsEmpty());

http://codereview.chromium.org/10444055/diff/4005/src/objects.h#newcode2565
src/objects.h:2565: bool mutable_transitions);
Turn this into an enum.

http://codereview.chromium.org/10444055/diff/4005/src/objects.h#newcode4792
src/objects.h:4792: MUST_USE_RESULT MaybeObject* CopyDropTransitions(bool
mutable_transitions);
Enum instead of bool

http://codereview.chromium.org/10444055/diff/4005/src/property.cc
File src/property.cc (right):

http://codereview.chromium.org/10444055/diff/4005/src/property.cc#newcode60
src/property.cc:60: PrintF(out, " -map:\n");
Make sure this debugging information is available somewhere.

[Toon Verwaest, 2012-06-04 09:17:48]

Addressed comments, please take a look

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/factory.cc
File src/factory.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/factory.cc#newc...
src/factory.cc:918: if (LinearSearchUnsorted(*result, *key, descriptor_count) ==
On 2012/06/01 13:49:55, danno wrote:
> Add a enum { EXPECT_SORTED, EXPECT_UNSORTED } and put both implementations in
> LinearSearch on the descriptor that take the enum selector as another
parameter.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/mark-compact.cc...
src/mark-compact.cc:1867: // printf("marking: %p\n",
static_cast<void*>(descriptors));
On 2012/06/01 13:49:55, danno wrote:
> Remove debugging code.

Done.

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

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects-inl.h#n...
src/objects-inl.h:1874: // immutable again.
On 2012/06/01 13:49:55, danno wrote:
> Re-enable this assert appropriately.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects-printer.cc
File src/objects-printer.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects-printer...
src/objects-printer.cc:275: break;
On 2012/06/01 13:49:55, danno wrote:
> Please add a printer for the transition map in the descriptor array so that
it's
> possible to output this information.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.cc#newc...
src/objects.cc:1571: {
On 2012/06/01 13:49:55, danno wrote:
> nit: No need for this whitespace change.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.cc#newc...
src/objects.cc:2321: // Only remember the map transition if the object's map is
NOT equal to
On 2012/06/01 13:49:55, danno wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.cc#newc...
src/objects.cc:4039: { MaybeObject* maybe = map()->CopyDropTransitions(false);
On 2012/06/01 13:49:55, danno wrote:
> This should be an enum rather than a bool, it's really difficult to see what
the
> parameter means at the usage site.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.cc#newc...
src/objects.cc:4945: if (descriptor_array_->MayContainTransitions())
As discussed offline, won't change since it is actually only related to
transitions, not so much to descriptors.

On 2012/06/01 13:49:55, danno wrote:
> This is confusing. What does descriptor iteration have to do with transition?
> Perhaps another predicate name, even if it does the same thing?

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.h#newco...
src/objects.h:2405: DECL_ACCESSORS(elements_transition, Map)
On 2012/06/01 13:49:55, danno wrote:
> probably should call this elements_transition_map

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.h#newco...
src/objects.h:2409: ASSERT(length() >= kFirstIndex || IsEmpty());
On 2012/06/01 13:49:55, danno wrote:
> ASSERT(length() > kFirstIndex || length() == kTransitionsIndex || IsEmpty());

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.h#newco...
src/objects.h:2565: bool mutable_transitions);
On 2012/06/01 13:49:55, danno wrote:
> Turn this into an enum.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/objects.h#newco...
src/objects.h:4792: MUST_USE_RESULT MaybeObject* CopyDropTransitions(bool
mutable_transitions);
On 2012/06/01 13:49:55, danno wrote:
> Enum instead of bool

Done.

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/property.cc
File src/property.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/4005/src/property.cc#new...
src/property.cc:60: PrintF(out, " -map:\n");
Since this function is only to be used directly, not transitively, it is not
necessary to move the code to a higher level.

On 2012/06/01 13:49:55, danno wrote:
> Make sure this debugging information is available somewhere.

[danno, 2012-06-04 13:52:13]

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/bootstrapper.c...
src/bootstrapper.cc:1635:
array_function->initial_map()->CopyDropTransitions(MAYBE_SHARED);
I think you mean MAY_BE_SHARED

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:174: enum SearchMode {
Since this is specific to Descriptor Arrays, consider making it an enum inside
the class, or call it something like DescriptorArraySearchMode.

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:183: MAYBE_SHARED,
MAY_BE_SHARED

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:184: FORCE_NEW
Either make this CANNOT_BE_SHARED, or, since this is behavior specific to
DescriptorArrays, make it an enum that's local to the class.

[Toon Verwaest, 2012-06-04 15:03:19]

Addressed comments, please take a look again.

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/bootstrapper.c...
src/bootstrapper.cc:1635:
array_function->initial_map()->CopyDropTransitions(MAYBE_SHARED);
On 2012/06/04 13:52:13, danno wrote:
> I think you mean MAY_BE_SHARED

Done.

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:174: enum SearchMode {
On 2012/06/04 13:52:13, danno wrote:
> Since this is specific to Descriptor Arrays, consider making it an enum inside
> the class, or call it something like DescriptorArraySearchMode.

Done.

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:183: MAYBE_SHARED,
On 2012/06/04 13:52:13, danno wrote:
> MAY_BE_SHARED

Done.

https://chromiumcodereview.appspot.com/10444055/diff/16001/src/objects.h#newc...
src/objects.h:184: FORCE_NEW
On 2012/06/04 13:52:13, danno wrote:
> Either make this CANNOT_BE_SHARED, or, since this is behavior specific to
> DescriptorArrays, make it an enum that's local to the class.

Done.

[danno, 2012-06-06 09:25:12]

LGTM with a few nits

http://codereview.chromium.org/10444055/diff/17024/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/10444055/diff/17024/src/objects.cc#newcode5843
src/objects.cc:5843: : CANNOT_BE_SHARED);
nit: strange formatting. Put the *_SHARED value into a temporary variable
immediately before.

http://codereview.chromium.org/10444055/diff/17024/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/10444055/diff/17024/src/objects.h#newcode173
src/objects.h:173: // descriptor array as input.
nit: remove the reference to descriptor if you want to make this a general enum.