[runtime] Add MapUpdater class that manages all kinds of map updates.

[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration
of a map to an up-to-date non-deprecated version.

BUG=v8:5495
Review-Url: https://codereview.chromium.org/2601643002
Cr-Commit-Position: refs/heads/master@{#42177}
Committed: https://chromium.googlesource.com/v8/v8/+/0f159f59736f20cb9fca128fed0ac1b29f6312e5

[Igor Sheludko, 2016-12-23 16:32:42]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2016-12-23 23:44:48]

Patchset #3 (id:60001) has been deleted

[Igor Sheludko, 2016-12-23 23:44:54]

Patchset #2 (id:40001) has been deleted

[Igor Sheludko, 2016-12-23 23:50:49]

Patchset #2 (id:80001) has been deleted

[Igor Sheludko, 2016-12-23 23:50:55]

Patchset #1 (id:20001) has been deleted

[Igor Sheludko, 2017-01-09 08:48:10]

Patchset #1 (id:100001) has been deleted

[Igor Sheludko, 2017-01-09 08:49:47]

Patchset #1 (id:120001) has been deleted

[Igor Sheludko, 2017-01-09 08:58:59]

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

[Igor Sheludko, 2017-01-09 08:58:59]

PTAL

[Igor Sheludko, 2017-01-09 08:59:03]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 08:59:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 09:27:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 09:27:43]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-01-09 13:01:19]

I didn't read all the code in detail since it's mostly the same afaict. Overall
it looks good. I don't really like the name of the class though, but that's
bikeshedding :)

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.cc
File src/map-reconfigurer.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.c...
src/map-reconfigurer.cc:123: if (FindRootMap()) return Result();
This is odd to read... I'd expect if (!FindRootMap()) return Result()
(Failure()?)

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.c...
src/map-reconfigurer.cc:156: state_ = kEnd;
these states are a little odd. That's just for debugging right?

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h
File src/map-reconfigurer.h (right):

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:20: // the reconfigured map and all the intermediate maps
are propely integrated
properly

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:30: //   virtually "enhanced" |old_map|'s descriptor
array (i.e. whose entry at
"updated"

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:33: // - Merge/generalize the "enhanced" descriptor array
of the |old_map| and
"updated"

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc#newcode...
src/objects.cc:4077: Handle<FieldType> Map::GeneralizeFieldTypes(Representation
rep1,
Why the rename? This is generalizing the type of a single field?

[Igor Sheludko, 2017-01-09 14:32:05]

Patchset #2 (id:160001) has been deleted

[Igor Sheludko, 2017-01-09 14:39:13]

Description was changed from

==========
[runtime] Add MapReconfigurer class that manages property reconfiguring.

BUG=v8:5495
==========

to

==========
[runtime] Add MapReconfigurer class that manages property reconfiguring.

BUG=v8:5495
==========

[Igor Sheludko, 2017-01-09 14:41:10]

Description was changed from

==========
[runtime] Add MapReconfigurer class that manages property reconfiguring.

BUG=v8:5495
==========

to

==========
[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration of a
map to an up-to-date non-deprecated version.

BUG=v8:5495
==========

[Igor Sheludko, 2017-01-09 14:41:31]

Description was changed from

==========
[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration of a
map to an up-to-date non-deprecated version.

BUG=v8:5495
==========

to

==========
[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration
of a map to an up-to-date non-deprecated version.

BUG=v8:5495
==========

[Igor Sheludko, 2017-01-09 14:53:46]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 14:53:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Igor Sheludko, 2017-01-09 14:54:26]

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.cc
File src/map-reconfigurer.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.c...
src/map-reconfigurer.cc:123: if (FindRootMap()) return Result();
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> This is odd to read... I'd expect if (!FindRootMap()) return Result()
> (Failure()?)

true means "done", therefore it returns. And all these functions never fail,
they just sometimes return generalized copy of a map.

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.c...
src/map-reconfigurer.cc:156: state_ = kEnd;
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> these states are a little odd. That's just for debugging right?

Right. And the updater instance should not be used twice. 

I can remove the state field if you don't like it.

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h
File src/map-reconfigurer.h (right):

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:20: // the reconfigured map and all the intermediate maps
are propely integrated
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> properly

Done.

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:30: //   virtually "enhanced" |old_map|'s descriptor
array (i.e. whose entry at
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> "updated"

Done.

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.h...
src/map-reconfigurer.h:33: // - Merge/generalize the "enhanced" descriptor array
of the |old_map| and
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> "updated"

Done.

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc#newcode...
src/objects.cc:4077: Handle<FieldType> Map::GeneralizeFieldTypes(Representation
rep1,
On 2017/01/09 13:01:19, Toon Verwaest wrote:
> Why the rename? This is generalizing the type of a single field?

Don't be confused, there is another Map::GeneralizeFieldType() right below which
does generalization of one field's type in given map. 
This method just merges two field types and it is commutative.

[commit-bot: I haz the power, 2017-01-09 15:35:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 15:35:08]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-01-10 10:28:08]

PTAL again.

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.cc
File src/map-reconfigurer.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/map-reconfigurer.c...
src/map-reconfigurer.cc:123: if (FindRootMap()) return Result();
On 2017/01/09 14:54:25, Igor Sheludko wrote:
> On 2017/01/09 13:01:19, Toon Verwaest wrote:
> > This is odd to read... I'd expect if (!FindRootMap()) return Result()
> > (Failure()?)
> 
> true means "done", therefore it returns. And all these functions never fail,
> they just sometimes return generalized copy of a map.

As discussed offline, these methods now return an updated state and we can check
it is equal to kEnd explicitly which should improve code readability.

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2601643002/diff/140001/src/objects.cc#newcode...
src/objects.cc:4077: Handle<FieldType> Map::GeneralizeFieldTypes(Representation
rep1,
On 2017/01/09 14:54:26, Igor Sheludko wrote:
> On 2017/01/09 13:01:19, Toon Verwaest wrote:
> > Why the rename? This is generalizing the type of a single field?
> 
> Don't be confused, there is another Map::GeneralizeFieldType() right below
which
> does generalization of one field's type in given map. 
> This method just merges two field types and it is commutative.

As discussed offline, I reverted this change and renamed the other method to
GeneralizeField().

[Igor Sheludko, 2017-01-10 10:28:14]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 10:28:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 10:57:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 10:57:30]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-01-10 12:22:21]

lgtm

[Igor Sheludko, 2017-01-10 12:41:07]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2017-01-10 12:41:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 12:42:56]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484052067435440,
"parent_rev": "8dfea24e3d8c7137ffae3bcaa24ad282b82485e5", "commit_rev":
"0f159f59736f20cb9fca128fed0ac1b29f6312e5"}

[commit-bot: I haz the power, 2017-01-10 12:43:00]

Description was changed from

==========
[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration
of a map to an up-to-date non-deprecated version.

BUG=v8:5495
==========

to

==========
[runtime] Add MapUpdater class that manages all kinds of map updates.

... including property reconfiguring, elements kind change and migration
of a map to an up-to-date non-deprecated version.

BUG=v8:5495

Review-Url: https://codereview.chromium.org/2601643002
Cr-Commit-Position: refs/heads/master@{#42177}
Committed:
https://chromium.googlesource.com/v8/v8/+/0f159f59736f20cb9fca128fed0ac1b29f6...
==========

[commit-bot: I haz the power, 2017-01-10 12:43:01]

Committed patchset #3 (id:200001) as
https://chromium.googlesource.com/v8/v8/+/0f159f59736f20cb9fca128fed0ac1b29f6...