Use map transitions when defining accessor properties.

Use map transitions when defining accessor properties.

AccessorPairs can now contain map transitions, which is similar to our current
handling of CONSTANT_FUNCTION/CONSTANT_TRANSITION, but generalized to a pair for
holding info about the getter and the setter. This way we can achieve map
sharing for objects with accessor properties, which is a prerequisite for making
them fast via inlining. We fall back to the previous way of handling accessor
properties when sharing is not possible or we don't handle a special case.

Note: When an exisiting accessor property is redefined we could in principle
move the AccessorPair out of the descriptor into the object itself (again just
like the way we do something similar for CONSTANT_FUNCTION/CONSTANT_TRANSITION),
but this would require a new property kind for holding a pair of values. Perhaps
we can implement this later, but for now this hopefully rare case is handled
like before, losing map sharing and potentially creating more maps than strictly
necessary.

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

[Michael Starzinger, 2012-05-02 13:30:14]

First round. A few nits and one real comment.

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

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4436: MaybeObject* getterOk = GetHeap()->undefined_value();
Can we move the result of "GetHeap()" into a local "heap" variable? Same goes
for the other methods below.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4649: return NewCallbackTrans(name, component, accessor,
attributes, accessors);
Can we add an assertion to the top of this method that
ASSERT(!accessor->IsMap()) to make sure we are not accidentally introducing
transitions with this method.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4657: MaybeObject* JSObject::CreateFreshAccessor(String* name,
Can we turn this method into a static helper that returns a "MaybeMap" to
transition to? The call to "set_map()" would be moved into the caller (i.e.
"DefineFastAccessor"). That way all this complexity would be hidden from the API
(i.e. JSObject) and there would only be one entry point to define fast JS-style
accessors.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4709: MaybeObject* JSObject::NewCallbackTrans(String* name,
Likewise. Also the full name "NewCallbackTransition" would be better.

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

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.h#newcode713
src/objects.h:713: class AccessorPair;
Can we alpha-sort this?

[Sven Panne, 2012-05-03 09:39:02]

Comments addressed, new CL uploaded...

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

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4436: MaybeObject* getterOk = GetHeap()->undefined_value();
On 2012/05/02 13:30:14, Michael Starzinger wrote:
> Can we move the result of "GetHeap()" into a local "heap" variable? Same goes
> for the other methods below.

Done for the occurrences where it is actually used more than once in a possible
control flow.

Rationale: I'd like to keep the mental context needed for understanding a piece
of code as small as possible, and every local variable works against that goal.
OTOH, speed considerations might make such a manual CSE sometimes necessary.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4649: return NewCallbackTrans(name, component, accessor,
attributes, accessors);
On 2012/05/02 13:30:14, Michael Starzinger wrote:
> Can we add an assertion to the top of this method that
> ASSERT(!accessor->IsMap()) to make sure we are not accidentally introducing
> transitions with this method.

I'll add a slightly stronger assertion, checking that the accessor is either a
spec function or undefined.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4657: MaybeObject* JSObject::CreateFreshAccessor(String* name,
On 2012/05/02 13:30:14, Michael Starzinger wrote:
> Can we turn this method into a static helper that returns a "MaybeMap" to
> transition to? The call to "set_map()" would be moved into the caller (i.e.
> "DefineFastAccessor"). That way all this complexity would be hidden from the
API
> (i.e. JSObject) and there would only be one entry point to define fast
JS-style
> accessors.

As discussed offline, let's just simply pass 'this' to keep changes in an atomic
way.

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.cc#newcode...
src/objects.cc:4709: MaybeObject* JSObject::NewCallbackTrans(String* name,
On 2012/05/02 13:30:14, Michael Starzinger wrote:
> Likewise. Also the full name "NewCallbackTransition" would be better.

It was because of our 80-column limit. :-) Done.

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

https://chromiumcodereview.appspot.com/10238005/diff/1/src/objects.h#newcode713
src/objects.h:713: class AccessorPair;
On 2012/05/02 13:30:14, Michael Starzinger wrote:
> Can we alpha-sort this?

Done.

[Michael Starzinger, 2012-05-03 12:23:34]

LGTM (with once comment).

https://chromiumcodereview.appspot.com/10238005/diff/5002/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10238005/diff/5002/src/objects.cc#newc...
src/objects.cc:4420: // accessors, not for the addition of each part.
It would really be nice to preserve this assertion somehow. In my understanding
the correct place to put it would be at the beginning of
DefinePropertyAccessor(). Unfortunately we don't have a lookup result there.

Maybe you could come up with some nifty way to check that assertion there.
Otherwise a comment there might be useful.

[Erik Corry, 2012-05-07 10:52:13]

This change regressed the context create benchmark 25% and increased the memory
use.  It would be nice to understand both issues.

[Michael Starzinger, 2012-05-08 12:47:59]

On 2012/05/07 10:52:13, Erik Corry wrote:
> This change regressed the context create benchmark 25% and increased the
memory
> use.  It would be nice to understand both issues.

Ouch, probably both related to what's serialized into the snapshot I guess.
Unfortunately Sven is on leave and I'm unsure when he will return.