Handle CALLBACKS with map transitions when doing GC in the map tree.

Handle CALLBACKS with map transitions when doing GC in the map tree.

This is yet anoter part of a long chain of CLs to achieve map sharing
when JavaScript accessors are used.

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

[Michael Starzinger, 2012-01-30 12:57:56]

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc
File src/objects.cc (left):

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#oldcode7295
src/objects.cc:7295: ASSERT(target->prototype() == this ||
Can we keep this assertion around? If we turn RestorePrototype into a member of
Map, this assertion can be placed in there.

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#oldcode7312
src/objects.cc:7312: ASSERT(target->prototype() == this ||
Likewise.

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#oldcode7321
src/objects.cc:7321: // If no map was found, make sure the FixedArray also gets
collected.
Can we keep this comment (and place it above line 7363 on the right side)?

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

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#newcode7287
src/objects.cc:7287: static bool RestoreProto(Object* object,
I would prefer "RestorePrototype" as a name. Also a comment of what the
semantics of keep_entry and the return value are, is needed here I think.

Just an idea: Since this is the diametrical opposite of
Map::CreateOneBackPointer, why not make it a member if Map and call it something
like RestoreOneBackPointer?

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#newcode7291
src/objects.cc:7291: Map* map = static_cast<Map*>(object);
If object->IsMap() is usable at this point, then Map::cast() is as well. I would
suggest using that instead of a static_cast<> here.

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#newcode7303
src/objects.cc:7303: // Live DescriptorArray objects will be marked, so we must
use low-level
The comment is no longer accurate.

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#newcode7305
src/objects.cc:7305: DescriptorArray* d = static_cast<DescriptorArray*>(
I think we can use our internal casts here. Could you please verify that?

https://chromiumcodereview.appspot.com/9225056/diff/1/src/objects.cc#newcode7310
src/objects.cc:7310: FixedArray* contents = static_cast<FixedArray*>(
Likewise.

[Sven Panne, 2012-01-30 13:34:39]

Incorporated review comments, PTAL.

Note that I'm currently run tons of test to make sure that we can really use
Foo::cast here, but I'm quite sure that this is the case...

http://codereview.chromium.org/9225056/diff/1/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#oldcode7295
src/objects.cc:7295: ASSERT(target->prototype() == this ||
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> Can we keep this assertion around? If we turn RestorePrototype into a member
of
> Map, this assertion can be placed in there.

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#oldcode7312
src/objects.cc:7312: ASSERT(target->prototype() == this ||
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#oldcode7321
src/objects.cc:7321: // If no map was found, make sure the FixedArray also gets
collected.
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> Can we keep this comment (and place it above line 7363 on the right side)?

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#newcode7287
src/objects.cc:7287: static bool RestoreProto(Object* object,
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> I would prefer "RestorePrototype" as a name. Also a comment of what the
> semantics of keep_entry and the return value are, is needed here I think.
> 
> Just an idea: Since this is the diametrical opposite of
> Map::CreateOneBackPointer, why not make it a member if Map and call it
something
> like RestoreOneBackPointer?

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#newcode7291
src/objects.cc:7291: Map* map = static_cast<Map*>(object);
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> If object->IsMap() is usable at this point, then Map::cast() is as well. I
would
> suggest using that instead of a static_cast<> here.

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#newcode7303
src/objects.cc:7303: // Live DescriptorArray objects will be marked, so we must
use low-level
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> The comment is no longer accurate.

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#newcode7305
src/objects.cc:7305: DescriptorArray* d = static_cast<DescriptorArray*>(
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> I think we can use our internal casts here. Could you please verify that?

Done.

http://codereview.chromium.org/9225056/diff/1/src/objects.cc#newcode7310
src/objects.cc:7310: FixedArray* contents = static_cast<FixedArray*>(
On 2012/01/30 12:57:56, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Starzinger, 2012-01-30 14:02:28]

LGTM.