Separating transitions from descriptors.

Separating transitions from descriptors.

In this design maps contain descriptor arrays, which in turn can contain transition arrays. If transitions are needed when no descriptor array is present, a descriptor array without real descriptors is inserted just so it can point at the transition array.

The transition array does not contain details about the field it transitions to. In order to weed out transitions to FIELDs from CONSTANT_FUNCTION (what used to be MAP_TRANSITION vs CONSTANT_TRANSITION), the transition needs to be followed and the details need to be looked up in the target map. CALLBACKS transitions are still easy to recognize since the transition targets are stored as an AccessorPair containing the maps, rather than the maps directly.

Currently AccessorPairs containing a transition and an accessor are shared between the descriptor array and the transition array. This simplifies lookup since we only have to look in one of both arrays. This will change in subsequent revisions, when descriptor arrays will become shared between multiple maps, since transitions cannot be shared.

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

[Toon Verwaest, 2012-06-28 10:03:41]

PTAL.

Michael: please have a look at transition array zapping and trimming (clear
non-live transitions).

[Michael Starzinger, 2012-06-28 16:02:12]

First round of drive-by comments.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1910:
base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key));
We can also use HeapObject::cast() here.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1965:
base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key));
Use HeapObject::cast() here.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1978: base_marker()->MarkObjectWithoutPush(value);
We only need to mark the slots if the pair itself wasn't marked. So move the
MarkObjectWithoutPush() into the condition.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:921: if (!IsTransitionArray()) return true;
Can this really happen? If not, drop this line.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:948: if (this == NULL) return true;
Can "this" really be NULL? If not, drop this line.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:956: continue;
Drop the continue.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:958: if (!CheckOneBackPointer(current_map, value)) return
false;
Can we turn this into a proper "else" branch and also assert that it actually is
a map in the else branch?

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:2001: if (!this->MayContainTransitions()) return NULL;
Somehow it feels wrong to return NULL here. Either return some empty transitions
array (e.g. EmptyFixedArray) or make the return type of this method "Object" and
return Smi::FromInt(0) here.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h
File src/transitions.h (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:49: inline Map* elements();
Can we call this elements_transition instead?

[Toon Verwaest, 2012-06-29 08:14:55]

Addressed Michaels comments.

- Renamed elements to elements_transition.
- Never return NULL for elements_transition and transitions(), but always let
the client check if the wanted value is present using HasElementsTransition or
HasTransitionArray.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1910:
base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key));
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> We can also use HeapObject::cast() here.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1965:
base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key));
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Use HeapObject::cast() here.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#ne...
src/mark-compact.cc:1978: base_marker()->MarkObjectWithoutPush(value);
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> We only need to mark the slots if the pair itself wasn't marked. So move the
> MarkObjectWithoutPush() into the condition.

Done.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:921: if (!IsTransitionArray()) return true;
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Can this really happen? If not, drop this line.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:948: if (this == NULL) return true;
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Can "this" really be NULL? If not, drop this line.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:956: continue;
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Drop the continue.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#n...
src/objects-debug.cc:958: if (!CheckOneBackPointer(current_map, value)) return
false;
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Can we turn this into a proper "else" branch and also assert that it actually
is
> a map in the else branch?

Done.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:2001: if (!this->MayContainTransitions()) return NULL;
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Somehow it feels wrong to return NULL here. Either return some empty
transitions
> array (e.g. EmptyFixedArray) or make the return type of this method "Object"
and
> return Smi::FromInt(0) here.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h
File src/transitions.h (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:49: inline Map* elements();
On 2012/06/28 16:02:12, Michael Starzinger wrote:
> Can we call this elements_transition instead?

Done.

[Jakob Kummerow, 2012-06-29 16:31:36]

I have some comments. These are for patch set 1; haven't looked at patch set 2
in detail yet.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:4994: // ASSERT(lookup->IsMapTransition());
Either enable or remove this, please.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:50: #include "transitions.h"
As an exception from not relying on indirect #includes, we usually don't
#include both X.h and X-inl.h anywhere, as we know for sure that X-inl.h
#includes X.h.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1920: // Perform a binary search in a fixed array.Low and high
are entry indices. If
nit: missing space after '.'.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1967: // SLOW_ASSERT(IsSortedNoDuplicates());
Please either (fix and) enable this or remove it.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:2013: void DescriptorArray::set_transitions(
nit: for declarations, each argument on its own line (if they don't all fit on
the first), please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3521: // transition array that will have place for an element
transition.
nit: 
// If the descriptor array does not have a transition array, install a new
// empty transition array that will have room for an elements transition.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3547: // If the map is using the empty descriptor array,
install a new empty
This comment is confusing. Maybe something like "If the map does not have a
descriptor array, create a new empty descriptor array."?

Also, why is this not called from AllowElementsTransition as a preparational
step?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3577: if (transitions_array == transitions()) {
You can just do ASSERT(transitions_array != transitions()) here.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1501: set_properties(FixedArray::cast(values));
While you're at it, you can get rid of the cast here by declaring "FixedArray*
values" in the first place.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1585: // We have accomplished the main goal, so return success.
Huh?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1637: Object* new_map;
Why not "Map* new_map"?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1654: // Do not add CONSTANT_TRANSITIONS to global objects
update comment?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1659: // Add a CONSTANT_TRANSITION descriptor to the old map,
update comment?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1668: MaybeObject* maybe_new_transitions =
Not sure how important it is, but so far we've been pretty consistent in putting
MaybeObjects into their own {}-block. Let's do that here (and below) too.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1671: // We have accomplished the main goal, so return success.
I'm not convinced that returning success here is reasonable, but let's leave it
as it is in this CL.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1675: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
see above.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1832: MaybeObject* maybe_new_transitions =
Again, please wrap in {}.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1839: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
Again.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2193: static void LookupTransition(Map* map,
Why is this a local static function? It would be more consistent if it were a
method of the Map class (or even inlined into LookupTransitionOrDescriptor).

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2307: MaybeObject* added_elements =
this->set_elements_transition_map(next_map);
Wrap in {} please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2938: // Is transition to CONSTANT_FUNCTION
nit: missing full stop.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4287: // TODO(verwaest) clean test
?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4621: MaybeObject* maybe_new_transitions =
Wrap in {} please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4624: // We have accomplished the main goal, so return success.
Comment is wrong.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4629: MaybeObject* transition_added =
map1->set_transitions(new_transitions);
again, {} please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4683: MaybeObject* maybe_transitions =
{}

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4688: MaybeObject* transition_added =
map2->set_transitions(new_transitions);
{}

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5060: *TransitionArrayHeader() = Smi::FromInt(0);
nit: {} please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5098: break;
This doesn't look right. Did you mean "continue"? As an alternative, remove this
statement, and put the "We definitely have no map transition" part into an "else
{...}" block.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5923: // Copy the descriptors, filtering out transitions and null
descriptors,
update comment?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:7350: return ClearBackPointer(heap,
t->GetValue(transition_index));
nit: indentation is off

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:7355: return getter && setter;
Shouldn't this be "getter || setter"?

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode1198
src/objects.h:1198: inline Address FieldAddress(int offset);
I guess you forgot to remove these declarations?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode2070
src/objects.h:2070: PrintElements(stdout);
Don't you mean PrintTransitions(stdout) here?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc
File src/property.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc#newcode95
src/property.cc:95: PrintF(out, " -type = call backs transition\n");
nit: callbacks is one word

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:31: #include "objects.h"
Again, #including objects.h is unnecessary if you #include objects-inl.h too.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:82: Map* transition_map, WriteBarrierMode mode) {
one line per argument, please.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:83: ASSERT(this != NULL);
can this happen?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:144: // TODO(verwaest) Currently details are always taken
from the getter if it
nit: missing colon ("// TODO(verwaest): ...")

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:175: if (this == NULL) return kNotFound;
can this happen?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc
File src/transitions.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#new...
src/transitions.cc:32: #include "transitions.h"
transitions-inl.h is enough.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#new...
src/transitions.cc:85: if (this == NULL) {
can this happen?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h
File src/transitions.h (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:46: // [length() - 2] Last transition
Is that "length() - 2" because kTransitionSize == 2? In that case, I'd write
"[length() - kTransitionSize]" instead.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:57: inline Object** GetKeySlot(int transition_number);
Set{Key,Value}Unchecked() -- what do we need these for?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:78: // Copy the transition array, inserting a new transition.
While adding, they
who's "they"?

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:120: // Is the transition array sorted and without duplicates?
nit: this comment is kinda unnecessary, as it doesn't add information beyond the
function name. Feel free to just drop it. 
(Same for the next two methods.)

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:151: static inline void NoIncrementalWriteBarrierSwap(
I don't see where this is used. If the intention was to forbid its use, please
indicate that in a comment. (You can remove the uninformative existing comment
in turn.)

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:155: inline void NoIncrementalWriteBarrierSwapTransitions(
I don't see where this is used.

[Michael Starzinger, 2012-07-05 12:55:51]

Another round of drive-by comments.

https://chromiumcodereview.appspot.com/10697015/diff/8001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://chromiumcodereview.appspot.com/10697015/diff/8001/src/transitions-inl...
src/transitions-inl.h:175: void TransitionArray::Set(int transition_number,
String* key, Object* value) {
This setter should take a WhitenessWitness as the last argument. This will
require a slight generalization of that class, because it is no longer specific
to the DescriptorArray.

https://chromiumcodereview.appspot.com/10697015/diff/8001/src/transitions-inl...
src/transitions-inl.h:178: key);
Actually I think it might be a good idea to pass the WhitenessWitness into
NoIncrementalWriteBarrierSet() as well, to catch these cases statically.

[Toon Verwaest, 2012-07-05 12:56:11]

Addressed comments. PTAL.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:4994: // ASSERT(lookup->IsMapTransition());
On 2012/06/29 16:31:36, Jakob wrote:
> Either enable or remove this, please.

Done.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:50: #include "transitions.h"
On 2012/06/29 16:31:36, Jakob wrote:
> As an exception from not relying on indirect #includes, we usually don't
> #include both X.h and X-inl.h anywhere, as we know for sure that X-inl.h
> #includes X.h.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1920: // Perform a binary search in a fixed array.Low and high
are entry indices. If
On 2012/06/29 16:31:36, Jakob wrote:
> nit: missing space after '.'.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1967: // SLOW_ASSERT(IsSortedNoDuplicates());
On 2012/06/29 16:31:36, Jakob wrote:
> Please either (fix and) enable this or remove it.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:2013: void DescriptorArray::set_transitions(
On 2012/06/29 16:31:36, Jakob wrote:
> nit: for declarations, each argument on its own line (if they don't all fit on
> the first), please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3521: // transition array that will have place for an element
transition.
On 2012/06/29 16:31:36, Jakob wrote:
> nit: 
> // If the descriptor array does not have a transition array, install a new
> // empty transition array that will have room for an elements transition.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3547: // If the map is using the empty descriptor array,
install a new empty
On 2012/06/29 16:31:36, Jakob wrote:
> This comment is confusing. Maybe something like "If the map does not have a
> descriptor array, create a new empty descriptor array."?
> 
> Also, why is this not called from AllowElementsTransition as a preparational
> step?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:3577: if (transitions_array == transitions()) {
On 2012/06/29 16:31:36, Jakob wrote:
> You can just do ASSERT(transitions_array != transitions()) here.

Done.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1501: set_properties(FixedArray::cast(values));
On 2012/06/29 16:31:36, Jakob wrote:
> While you're at it, you can get rid of the cast here by declaring "FixedArray*
> values" in the first place.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1585: // We have accomplished the main goal, so return success.
On 2012/06/29 16:31:36, Jakob wrote:
> Huh?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1637: Object* new_map;
On 2012/06/29 16:31:36, Jakob wrote:
> Why not "Map* new_map"?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1654: // Do not add CONSTANT_TRANSITIONS to global objects
On 2012/06/29 16:31:36, Jakob wrote:
> update comment?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1659: // Add a CONSTANT_TRANSITION descriptor to the old map,
On 2012/06/29 16:31:36, Jakob wrote:
> update comment?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1668: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
> Not sure how important it is, but so far we've been pretty consistent in
putting
> MaybeObjects into their own {}-block. Let's do that here (and below) too.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1671: // We have accomplished the main goal, so return success.
On 2012/06/29 16:31:36, Jakob wrote:
> I'm not convinced that returning success here is reasonable, but let's leave
it
> as it is in this CL.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1675: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
> see above.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1832: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
> Again, please wrap in {}.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:1839: MaybeObject* transition_added =
old_map->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
> Again.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2193: static void LookupTransition(Map* map,
On 2012/06/29 16:31:36, Jakob wrote:
> Why is this a local static function? It would be more consistent if it were a
> method of the Map class (or even inlined into LookupTransitionOrDescriptor).

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2307: MaybeObject* added_elements =
this->set_elements_transition_map(next_map);
On 2012/06/29 16:31:36, Jakob wrote:
> Wrap in {} please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:2938: // Is transition to CONSTANT_FUNCTION
On 2012/06/29 16:31:36, Jakob wrote:
> nit: missing full stop.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4287: // TODO(verwaest) clean test
On 2012/06/29 16:31:36, Jakob wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4621: MaybeObject* maybe_new_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
> Wrap in {} please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4624: // We have accomplished the main goal, so return success.
On 2012/06/29 16:31:36, Jakob wrote:
> Comment is wrong.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4629: MaybeObject* transition_added =
map1->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
> again, {} please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4683: MaybeObject* maybe_transitions =
On 2012/06/29 16:31:36, Jakob wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:4688: MaybeObject* transition_added =
map2->set_transitions(new_transitions);
On 2012/06/29 16:31:36, Jakob wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5060: *TransitionArrayHeader() = Smi::FromInt(0);
On 2012/06/29 16:31:36, Jakob wrote:
> nit: {} please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5098: break;
On 2012/06/29 16:31:36, Jakob wrote:
> This doesn't look right. Did you mean "continue"? As an alternative, remove
this
> statement, and put the "We definitely have no map transition" part into an
"else
> {...}" block.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:5923: // Copy the descriptors, filtering out transitions and null
descriptors,
On 2012/06/29 16:31:36, Jakob wrote:
> update comment?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:7350: return ClearBackPointer(heap,
t->GetValue(transition_index));
On 2012/06/29 16:31:36, Jakob wrote:
> nit: indentation is off

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.cc#newcode...
src/objects.cc:7355: return getter && setter;
No. Getter and setter are booleans indicating that their related component was
cleared. Only if both components are cleared we can clear the whole pair.

On 2012/06/29 16:31:36, Jakob wrote:
> Shouldn't this be "getter || setter"?

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode1198
src/objects.h:1198: inline Address FieldAddress(int offset);
On 2012/06/29 16:31:36, Jakob wrote:
> I guess you forgot to remove these declarations?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects.h#newcode2070
src/objects.h:2070: PrintElements(stdout);
On 2012/06/29 16:31:36, Jakob wrote:
> Don't you mean PrintTransitions(stdout) here?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc
File src/property.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/property.cc#newcode95
src/property.cc:95: PrintF(out, " -type = call backs transition\n");
On 2012/06/29 16:31:36, Jakob wrote:
> nit: callbacks is one word

Done.

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

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:31: #include "objects.h"
On 2012/06/29 16:31:36, Jakob wrote:
> Again, #including objects.h is unnecessary if you #include objects-inl.h too.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:82: Map* transition_map, WriteBarrierMode mode) {
On 2012/06/29 16:31:36, Jakob wrote:
> one line per argument, please.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:83: ASSERT(this != NULL);
On 2012/06/29 16:31:36, Jakob wrote:
> can this happen?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:144: // TODO(verwaest) Currently details are always taken
from the getter if it
On 2012/06/29 16:31:36, Jakob wrote:
> nit: missing colon ("// TODO(verwaest): ...")

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions-inl.h#...
src/transitions-inl.h:175: if (this == NULL) return kNotFound;
On 2012/06/29 16:31:36, Jakob wrote:
> can this happen?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc
File src/transitions.cc (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#new...
src/transitions.cc:32: #include "transitions.h"
On 2012/06/29 16:31:36, Jakob wrote:
> transitions-inl.h is enough.

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.cc#new...
src/transitions.cc:85: if (this == NULL) {
On 2012/06/29 16:31:36, Jakob wrote:
> can this happen?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h
File src/transitions.h (right):

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:46: // [length() - 2] Last transition
No, the comment was out of date. It's length() - 1, because there's the elements
transition. It'll be - 2 once the prototype transition is in; and - 3 once the
descriptor array is in as well.

On 2012/06/29 16:31:36, Jakob wrote:
> Is that "length() - 2" because kTransitionSize == 2? In that case, I'd write
> "[length() - kTransitionSize]" instead.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:57: inline Object** GetKeySlot(int transition_number);
On 2012/06/29 16:31:36, Jakob wrote:
> Set{Key,Value}Unchecked() -- what do we need these for?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:78: // Copy the transition array, inserting a new transition.
While adding, they
On 2012/06/29 16:31:36, Jakob wrote:
> who's "they"?

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:120: // Is the transition array sorted and without duplicates?
On 2012/06/29 16:31:36, Jakob wrote:
> nit: this comment is kinda unnecessary, as it doesn't add information beyond
the
> function name. Feel free to just drop it. 
> (Same for the next two methods.)

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:151: static inline void NoIncrementalWriteBarrierSwap(
On 2012/06/29 16:31:36, Jakob wrote:
> I don't see where this is used. If the intention was to forbid its use, please
> indicate that in a comment. (You can remove the uninformative existing comment
> in turn.)

Done.

https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newc...
src/transitions.h:155: inline void NoIncrementalWriteBarrierSwapTransitions(
On 2012/06/29 16:31:36, Jakob wrote:
> I don't see where this is used.

Done.

[Toon Verwaest, 2012-07-05 13:32:36]

Moved the WhitnessWitness class to FixedArray, and used it in the
TransitionArray code as well.

[Jakob Kummerow, 2012-07-05 13:41:45]

LGTM if you fix the comment we discussed offline.

[Michael Starzinger, 2012-07-05 13:46:56]

Looks OK (but I just looked at the GC related stuff, I'll leave the final
decision up to Jakob).