Property reconfiguring implemented. Tests added.

Property reconfiguring implemented.

Previous approach for property reconfiguration was to create a free-floating map with generalized representations of all fields. This patch does it right.

When property is reconfigured either by changing its kind (kData <-> kAccessor) or its attributes it implies creation of a new branch in transition tree. If such a branch already existed before reconfiguration then it should be merged with the old (or source) branch of the transition tree. Merging procedure includes all the heavy machinery such as property location changes (kDescriptor -> kField), field representation/field type generalization, map deprecation, etc.

Committed: https://crrev.com/35841b505570a14793c64f86c5fe7c57542dd8b5
Cr-Commit-Position: refs/heads/master@{#26667}

[Igor Sheludko, 2015-01-29 16:28:00]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-01-29 16:28:14]

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

[Igor Sheludko, 2015-01-29 16:28:15]

PTAL

[Toon Verwaest, 2015-01-30 13:02:53]

Looking good. Merging some of the very similar tests would be nice though... I
presume it can be reduced by quite a few lines of testcode, which improves
future maintainability.

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2415
src/objects.cc:2415: next_location = store_mode == FORCE_FIELD ? kField :
kDescriptor;
isn't this guaranteed above?

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2466
src/objects.cc:2466: } else {
else if

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2569
src/objects.cc:2569: PropertyKind merged_kind = target_details.kind();
I wouldn't call this merged_, since you aren't merging data; rather choosing
between the current or new_kind.

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2591
src/objects.cc:2591: merged_attributes = new_attributes;
Same as above

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2639
src/objects.cc:2639: PropertyAttributes merged_attributes =
old_details.attributes();
Same for all above

https://codereview.chromium.org/888623002/diff/20001/test/cctest/test-migrati...
File test/cctest/test-migrations.cc (right):

https://codereview.chromium.org/888623002/diff/20001/test/cctest/test-migrati...
test/cctest/test-migrations.cc:686: }
Seems like you could merge the few tests above into a single one that has arrays
of [[start_repr, new_repr]*]. If you add "deprecated" you can also handle
none->* cases.

https://codereview.chromium.org/888623002/diff/20001/test/cctest/test-migrati...
test/cctest/test-migrations.cc:1122:
TEST(ReconfigureDataFieldAttribute_GeneralizeRepresentationSmiToDouble) {
Same here?

[Igor Sheludko, 2015-02-11 15:58:13]

Patchset #3 (id:60001) has been deleted

[Igor Sheludko, 2015-02-11 16:02:49]

Patchset #3 (id:80001) has been deleted

[Igor Sheludko, 2015-02-11 16:06:01]

I addressed comments, refactored tests and added more of them. 

I also slightly changed behavior in case of property kind reconfiguring (see
ReconfigureAccessorToNonExistingDataField* tests) which caused changes in
LookupIterator.

PTAL

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2415
src/objects.cc:2415: next_location = store_mode == FORCE_FIELD ? kField :
kDescriptor;
On 2015/01/30 13:02:53, Toon Verwaest wrote:
> isn't this guaranteed above? 

Done.

https://codereview.chromium.org/888623002/diff/20001/src/objects.cc#newcode2466
src/objects.cc:2466: } else {
On 2015/01/30 13:02:53, Toon Verwaest wrote:
> else if

Done.

[adamk, 2015-02-11 19:27:45]

This work is super-exciting; would you mind filling out the CL description to
describe more of what's in this patch?

[Igor Sheludko, 2015-02-12 09:13:36]

On 2015/02/11 19:27:45, adamk wrote:
> This work is super-exciting; would you mind filling out the CL description to
> describe more of what's in this patch?

Done.

[Toon Verwaest, 2015-02-16 11:36:09]

lgtm

[Igor Sheludko, 2015-02-16 11:52:21]

New patchsets have been uploaded after l-g-t-m from verwaest@chromium.org

[Igor Sheludko, 2015-02-16 13:37:53]

Patchset #6 (id:160001) has been deleted

[Igor Sheludko, 2015-02-16 15:23:25]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-02-16 15:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888623002/200001

[commit-bot: I haz the power, 2015-02-16 15:25:40]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-02-16 15:26:06]

Patchset 7 (id:??) landed as
https://crrev.com/35841b505570a14793c64f86c5fe7c57542dd8b5
Cr-Commit-Position: refs/heads/master@{#26667}