Handlefy Descriptor and other code in objects.cc

Handlefy Descriptor and other code in objects.cc

R=verwaest@chromium.org

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

[Toon Verwaest, 2014-04-08 12:58:54]

https://codereview.chromium.org/228333003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode3153
src/objects.cc:3153: static void Insert(Name* key,
Can't call from unhandlified into handlified code!

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode3329
src/objects.cc:3329: 
DisallowHeapAllocation?

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode4711
src/objects.cc:4711: object->set_map(*map);
JSObject::SetMapAndElements

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6793
src/objects.cc:6793: new_descriptors->Append(descriptor);
We cannot GC anymore after this has happened. Otherwise it may be that the newly
appended descriptor isn't properly marked as it is weak until
result->InitializeDescriptors(*new_descriptors);

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6818
src/objects.cc:6818: Map* walk_map;
DisallowHeapAllocation no_gc from here down.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6856
src/objects.cc:6856: MaybeObject*
Map::CopyReplaceDescriptorsFull(DescriptorArray* descriptors,
This name doesn't really make sense.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6882
src/objects.cc:6882: Handle<Map> Map::CopyReplaceDescriptorsFull(Handle<Map>
map,
Can we get rid of this version of the method? Kinda ugly to have both versions;
and this name doesn't really make sense.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode7198
src/objects.cc:7198: new_descriptors->Set(i, descriptor, witness);
In this Set case, the "pointer" will be wrong.

Can we just use CopyUpTo rather than this local copy; and afterwards do a "set"
which guarantees that the "pointer" value is maintained?

https://codereview.chromium.org/228333003/diff/1/src/transitions.cc
File src/transitions.cc (right):

https://codereview.chromium.org/228333003/diff/1/src/transitions.cc#newcode209
src/transitions.cc:209: for (int i = 0; i < number_of_transitions; ++i) {
Wrong. number_of_transitions can change because of
TransitionArray::AllocateHandle; since it's a weak array.

[mvstanton, 2014-04-09 09:43:20]

Hi Toon,
Thanks for the great comments, I've addressed all but one where I have a
question (JSObject::ResetElements() change).
thx,
--Michael

https://codereview.chromium.org/228333003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode3153
src/objects.cc:3153: static void Insert(Name* key,
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> Can't call from unhandlified into handlified code!

Fixed.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode3329
src/objects.cc:3329: 
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> DisallowHeapAllocation?

Done.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode4711
src/objects.cc:4711: object->set_map(*map);
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> JSObject::SetMapAndElements

But initialize_elements() contains detailed logic to determine the elements
given a map. Why should I use SetMapAndElements()?

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6793
src/objects.cc:6793: new_descriptors->Append(descriptor);
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> We cannot GC anymore after this has happened. Otherwise it may be that the
newly
> appended descriptor isn't properly marked as it is weak until
> result->InitializeDescriptors(*new_descriptors);

Okay, I re-ordered things for the desired effect.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6818
src/objects.cc:6818: Map* walk_map;
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> DisallowHeapAllocation no_gc from here down.

I've got it above now, I think that is fine?

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6856
src/objects.cc:6856: MaybeObject*
Map::CopyReplaceDescriptorsFull(DescriptorArray* descriptors,
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> This name doesn't really make sense.

Already gone.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode6882
src/objects.cc:6882: Handle<Map> Map::CopyReplaceDescriptorsFull(Handle<Map>
map,
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> Can we get rid of this version of the method? Kinda ugly to have both
versions;
> and this name doesn't really make sense.

Already gone.

https://codereview.chromium.org/228333003/diff/1/src/objects.cc#newcode7198
src/objects.cc:7198: new_descriptors->Set(i, descriptor, witness);
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> In this Set case, the "pointer" will be wrong.
> 
> Can we just use CopyUpTo rather than this local copy; and afterwards do a
"set"
> which guarantees that the "pointer" value is maintained?

I followed your suggestion to just add 
descriptor->SetSortedKeyIndex(descriptors->GetSortedKeyIndex(i));
before the Set() call.

https://codereview.chromium.org/228333003/diff/1/src/transitions.cc
File src/transitions.cc (right):

https://codereview.chromium.org/228333003/diff/1/src/transitions.cc#newcode209
src/transitions.cc:209: for (int i = 0; i < number_of_transitions; ++i) {
On 2014/04/08 12:58:55, Toon Verwaest wrote:
> Wrong. number_of_transitions can change because of
> TransitionArray::AllocateHandle; since it's a weak array.

Done.

[mvstanton, 2014-04-09 11:43:05]

Okay, here is an update with the use of SetMapAndElements() in
JSObject::ResetElements(). PTAL, thx,
--Michael

[Toon Verwaest, 2014-04-09 13:49:24]

Overall LGTM. Main style comments:
- clean-up how you overflow arguments to fit what we generally do in V8 (see
inline comment)
- Replace Handle<...> value = handle(...) with Handle<...> value(...);

https://codereview.chromium.org/228333003/diff/60001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/228333003/diff/60001/src/elements.cc#newcode1461
src/elements.cc:1461: MUST_USE_RESULT static Handle<Object>
SetLengthWithoutNormalize(
Remove MUST_USE_RESULT

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3163
src/objects.cc:3163: Handle<AccessorInfo> entry =
handle(AccessorInfo::cast(callbacks->get(i)));
Handle<AccessorInfo> entry(AccessorInfo::cast(...));

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3164
src/objects.cc:3164: Handle<Name> key = handle(Name::cast(entry->name()));
Same.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3380
src/objects.cc:3380: Handle<Map> start_map = handle(object->map());
Handle<Map> start_map(object->map());

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3410
src/objects.cc:3410: Handle<Map> closest_map =
handle(FindClosestElementsTransition(*map, kind));
closest_map(FindClosest...);

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode4771
src/objects.cc:4771: Handle<FixedArrayBase> elements =
handle(map->GetInitialElements());
elements(map->GetInitial...)

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode6861
src/objects.cc:6861: // Disallow gc until after result->InitializeDescriptors()
is called.
Splitting this up isn't necessary (doesn't help). We just need to
disallow-heap-allocation between new_descriptors->Append in the other branch;
and InitializeDescriptors below. Perhaps it makes sense to rather copy
result->SetBackPointer(*map) and result->InitializeDescriptors(*new_descriptors)
into both branches; and have a DisallowHeapAllocation in the true case before
Append.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7057
src/objects.cc:7057: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
descriptors(map->instance..);

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7097
src/objects.cc:7097: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
descriptors(map->...)

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7138
src/objects.cc:7138: Handle<DescriptorArray> old_descriptors =
handle(map->instance_descriptors());
old_descriptors(map->...)

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7146
src/objects.cc:7146: return Map::CopyReplaceDescriptor(map, old_descriptors,
descriptor,
nit: In general, whenever arguments overflow, either put one per line, or break
after the (, as:
Map::CopyReplaceDescriptor(
   map, old_descriptors, ...);

It just makes the code more readable if we follow the same pattern everywhere...

[mvstanton, 2014-04-09 14:26:06]

Thanks for the review!

https://codereview.chromium.org/228333003/diff/60001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/228333003/diff/60001/src/elements.cc#newcode1461
src/elements.cc:1461: MUST_USE_RESULT static Handle<Object>
SetLengthWithoutNormalize(
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> Remove MUST_USE_RESULT

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3163
src/objects.cc:3163: Handle<AccessorInfo> entry =
handle(AccessorInfo::cast(callbacks->get(i)));
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> Handle<AccessorInfo> entry(AccessorInfo::cast(...));

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3164
src/objects.cc:3164: Handle<Name> key = handle(Name::cast(entry->name()));
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> Same.

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3339
src/objects.cc:3339: INSERT_TRANSITION);
Put arguments all on a new line.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3346
src/objects.cc:3346: INSERT_TRANSITION);
Put arguments all on a new line.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3357
src/objects.cc:3357: Handle<Map> current_map = handle(object->map());
Fixed here to current_map(object->map), and other places.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3380
src/objects.cc:3380: Handle<Map> start_map = handle(object->map());
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> Handle<Map> start_map(object->map());

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3410
src/objects.cc:3410: Handle<Map> closest_map =
handle(FindClosestElementsTransition(*map, kind));
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> closest_map(FindClosest...);

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode4771
src/objects.cc:4771: Handle<FixedArrayBase> elements =
handle(map->GetInitialElements());
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> elements(map->GetInitial...)

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode6861
src/objects.cc:6861: // Disallow gc until after result->InitializeDescriptors()
is called.
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> Splitting this up isn't necessary (doesn't help). We just need to
> disallow-heap-allocation between new_descriptors->Append in the other branch;
> and InitializeDescriptors below. Perhaps it makes sense to rather copy
> result->SetBackPointer(*map) and
result->InitializeDescriptors(*new_descriptors)
> into both branches; and have a DisallowHeapAllocation in the true case before
> Append.

Sounds good, done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode6914
src/objects.cc:6914: return Map::CopyReplaceDescriptors(map, descriptors, flag,
Put all args on a new line.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7057
src/objects.cc:7057: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> descriptors(map->instance..);

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7097
src/objects.cc:7097: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> descriptors(map->...)

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7131
src/objects.cc:7131: key, SIMPLE_TRANSITION);
Args all on a new line.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7138
src/objects.cc:7138: Handle<DescriptorArray> old_descriptors =
handle(map->instance_descriptors());
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> old_descriptors(map->...)

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7146
src/objects.cc:7146: return Map::CopyReplaceDescriptor(map, old_descriptors,
descriptor,
On 2014/04/09 13:49:24, Toon Verwaest wrote:
> nit: In general, whenever arguments overflow, either put one per line, or
break
> after the (, as:
> Map::CopyReplaceDescriptor(
>    map, old_descriptors, ...);
> 
> It just makes the code more readable if we follow the same pattern
everywhere...

Done.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7239
src/objects.cc:7239: simple_flag);
All args on a new line.

https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode9795
src/objects.cc:9795: INSERT_TRANSITION);
Args all on new line.

https://codereview.chromium.org/228333003/diff/60001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/228333003/diff/60001/src/runtime.cc#newcode2965
src/runtime.cc:2965: Handle<Map> new_map = Map::CopyReplaceDescriptor(map,
All args on a new line.

[mvstanton, 2014-04-09 14:26:54]

Committed patchset #4 manually as r20628 (presubmit successful).