TransitionArray now uses <is_data_property, name, attributes> tuple as a key, which allows to have …

TransitionArray now uses <is_data_property, name, attributes> tuple as a key, which allows to have several entries for the same property name.

R=verwaest@chromium.org

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

[Igor Sheludko, 2014-10-20 08:20:08]

PTAL

[Igor Sheludko, 2014-10-23 09:08:54]

ishell@chromium.org changed reviewers:
+ verwaest@chromium.org

[Igor Sheludko, 2014-10-23 09:08:55]

PTAL

[Igor Sheludko, 2014-10-23 17:36:13]

Patchset #2 (id:20001) has been deleted

[Toon Verwaest, 2014-10-24 06:43:34]

You'll have to rebase on top of the CL that grows transition arrays...

https://codereview.chromium.org/661133002/diff/40001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://codereview.chromium.org/661133002/diff/40001/src/transitions-inl.h#ne...
src/transitions-inl.h:153: bool TransitionArray::IsSpecialTransition(Name* name)
{
Please get rid of this method; instead make it dependent on how the search is
called.

Even for "IsSorted.." you shouldn't need it. There should never be multiple
entries for SpecialTransition names, so we shouldn't ensure that the details are
different.

[Igor Sheludko, 2014-11-04 10:05:01]

PTAL

https://codereview.chromium.org/661133002/diff/40001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://codereview.chromium.org/661133002/diff/40001/src/transitions-inl.h#ne...
src/transitions-inl.h:153: bool TransitionArray::IsSpecialTransition(Name* name)
{
On 2014/10/24 06:43:34, Toon Verwaest wrote:
> Please get rid of this method; instead make it dependent on how the search is
> called.
> 
> Even for "IsSorted.." you shouldn't need it. There should never be multiple
> entries for SpecialTransition names, so we shouldn't ensure that the details
are
> different.

Done. Now I'm using SimpleTransitionFlag as a to detect the kind of transition
and this method is left here only for checking purposes (DEBUG mode only).

[Toon Verwaest, 2014-11-04 10:50:51]

https://codereview.chromium.org/661133002/diff/80001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions-inl.h#ne...
src/transitions-inl.h:217: DCHECK_NE(kNotFound, descriptor);
Mmh, this doesn't seem necessary. We shouldn't need to compare details ever when
you can't find it at the end. This is only true if you added a setter/getter to
an existing setter/getter. In such case, that's the only possible transition
under that name. If we are fetching such details, we're probably computing too
much stuff.

https://codereview.chromium.org/661133002/diff/80001/src/transitions.cc
File src/transitions.cc (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions.cc#newco...
src/transitions.cc:199: for (;;) {
for ( ; transition < nof_transitions && GetKey(transition) == key; transition++)
?

https://codereview.chromium.org/661133002/diff/80001/src/transitions.h
File src/transitions.h (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions.h#newcod...
src/transitions.h:223: // 0 if set1 equal to set2 and 1 otherwise.
Sets? It just compares 2 keys with extra info than just the name...

[Igor Sheludko, 2014-11-04 16:44:16]

https://codereview.chromium.org/661133002/diff/80001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions-inl.h#ne...
src/transitions-inl.h:217: DCHECK_NE(kNotFound, descriptor);
On 2014/11/04 10:50:50, Toon Verwaest wrote:
> Mmh, this doesn't seem necessary. We shouldn't need to compare details ever
when
> you can't find it at the end. This is only true if you added a setter/getter
to
> an existing setter/getter. In such case, that's the only possible transition
> under that name. If we are fetching such details, we're probably computing too
> much stuff.

Done. Now transitions always created for last added property. I had to disabl
the ability to reconfigure non-last accessor property. I'll address
reconfiguration in next CL.

https://codereview.chromium.org/661133002/diff/80001/src/transitions.cc
File src/transitions.cc (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions.cc#newco...
src/transitions.cc:199: for (;;) {
On 2014/11/04 10:50:50, Toon Verwaest wrote:
> for ( ; transition < nof_transitions && GetKey(transition) == key;
transition++)
> ?

Done.

https://codereview.chromium.org/661133002/diff/80001/src/transitions.h
File src/transitions.h (right):

https://codereview.chromium.org/661133002/diff/80001/src/transitions.h#newcod...
src/transitions.h:223: // 0 if set1 equal to set2 and 1 otherwise.
On 2014/11/04 10:50:50, Toon Verwaest wrote:
> Sets? It just compares 2 keys with extra info than just the name...

Done.

[Toon Verwaest, 2014-11-05 09:05:43]

lgtm

[Igor Sheludko, 2014-11-05 09:26:38]

Committed patchset #5 (id:100001) manually as 25136 (presubmit successful).