Avoid sharing AccessorPairs during Genesis.

Avoid sharing AccessorPairs during Genesis.

To test the upcoming changes for map sharing in the presence of accessors, it is
essential that we keep a few global invariants: The map tree should always stay
a tree and AccessorPairs should not be shared between different DescriptorArrays
and/or StringDictionaries. This CL adds a test method for the latter invariant
and makes some changes to the bootstrapping process to avoid such sharing.

Note that we can't enable the new test method permanently yet, because we
currently go back and forth between fast mode and slow mode when adding an
accessor and break this invariant temporarily. This will be handled in a
separate CL.

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

[Michael Starzinger, 2012-02-17 13:33:27]

LGTM (with comments addressed).

https://chromiumcodereview.appspot.com/9417043/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://chromiumcodereview.appspot.com/9417043/diff/1/src/bootstrapper.cc#new...
src/bootstrapper.cc:177: void InitArgumentsAndCaller(Handle<Map> map);
In ECMA-Script speak this method should better be called
PoisonArgumentsAndCaller().

https://chromiumcodereview.appspot.com/9417043/diff/1/src/bootstrapper.cc#new...
src/bootstrapper.cc:556: Handle<AccessorPair>
arguments(factory()->NewAccessorPair());
Could you rewrite this as using the assignment operator so that it is in sync
with the blocks above and below?

https://chromiumcodereview.appspot.com/9417043/diff/1/src/bootstrapper.cc#new...
src/bootstrapper.cc:563: Handle<AccessorPair>
caller(factory()->NewAccessorPair());
Likewise.

https://chromiumcodereview.appspot.com/9417043/diff/1/src/bootstrapper.cc#new...
src/bootstrapper.cc:644: Handle<JSFunction> throw_function =
This is now unused, remove it.

https://chromiumcodereview.appspot.com/9417043/diff/1/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/9417043/diff/1/src/heap.cc#newcode5075
src/heap.cc:5075: // initial state. We use the LSB of the AccessorPair's getter
as the marking
Actually we want to use the "Smi tag" which happens to be the "LSB".

https://chromiumcodereview.appspot.com/9417043/diff/1/src/heap.cc#newcode5087
src/heap.cc:5087: uintptr_t after = (phase == 0) ? (before & ~1) : (before | 1);
Please use the kHeapObjectTag[Mask] and kSmiTag[Mask] constants here to do the
masking. Also HasHeapObjectTag() and HasSmiTag() might come in handy.

[Sven Panne, 2012-02-20 08:26:03]

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc#newcode177
src/bootstrapper.cc:177: void InitArgumentsAndCaller(Handle<Map> map);
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> In ECMA-Script speak this method should better be called
> PoisonArgumentsAndCaller().

Done.

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc#newcode556
src/bootstrapper.cc:556: Handle<AccessorPair>
arguments(factory()->NewAccessorPair());
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> Could you rewrite this as using the assignment operator so that it is in sync
> with the blocks above and below?

As discussed offline, I'll do it the other way round. Furthermore, I'll sync
this with the non-strict version of this method.

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc#newcode563
src/bootstrapper.cc:563: Handle<AccessorPair>
caller(factory()->NewAccessorPair());
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> Likewise.

See above.

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc#newcode644
src/bootstrapper.cc:644: Handle<JSFunction> throw_function =
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> This is now unused, remove it.

Done.

http://codereview.chromium.org/9417043/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/9417043/diff/1/src/heap.cc#newcode5075
src/heap.cc:5075: // initial state. We use the LSB of the AccessorPair's getter
as the marking
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> Actually we want to use the "Smi tag" which happens to be the "LSB".

Done.

http://codereview.chromium.org/9417043/diff/1/src/heap.cc#newcode5087
src/heap.cc:5087: uintptr_t after = (phase == 0) ? (before & ~1) : (before | 1);
On 2012/02/17 13:33:28, Michael Starzinger wrote:
> Please use the kHeapObjectTag[Mask] and kSmiTag[Mask] constants here to do the
> masking. Also HasHeapObjectTag() and HasSmiTag() might come in handy.

Done.

[Kevin Millikin (Chromium), 2012-02-20 09:30:19]

Drive by comment.

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/9417043/diff/1/src/bootstrapper.cc#newcode556
src/bootstrapper.cc:556: Handle<AccessorPair>
arguments(factory()->NewAccessorPair());
The existing V8 style is to prefer '=' notation for constructors that are
(essentially) a copy (e.g., the pointer assignment Handle constructors) and
constructor call notation for ones that are doing some sort of allocation or
other work in the constructor itself (e.g., the HandleScope::CreateHandle Handle
constructors).

We've been pretty consistent about that, so you'll have to change a lot of sites
if you think it's worth changing.