Refactor Map::ConstructionCount.

Refactor Map::ConstructionCount.

This combines Map::DoneInobjectSlackTracking and Map::ConstructionCount into one more generic 4-bit counter.

Counter values from 15 down to 8 are used for in-object slack tracking, values from 7 down to 0 are free to be used for a new counter when in-object slack tracking is inactive.

[ulan, 2014-12-02 11:30:51]

Patchset #1 (id:1) has been deleted

[ulan, 2014-12-02 11:30:58]

Patchset #1 (id:20001) has been deleted

[ulan, 2014-12-02 11:31:07]

Patchset #1 (id:40001) has been deleted

[ulan, 2014-12-02 11:36:34]

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

[ulan, 2014-12-02 11:41:05]

Igor, please take a look. Toon, this is fyi.

https://codereview.chromium.org/767253002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/767253002/diff/60001/src/objects.h#newcode5671
src/objects.h:5671: static const int kRetainingCounterStart =
kSlackTrackingCounterEnd - 1;
This counter will be used in future CL.

[Igor Sheludko, 2014-12-04 10:11:09]

https://codereview.chromium.org/767253002/diff/80001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/767253002/diff/80001/src/heap/heap.cc#newcode...
src/heap/heap.cc:2360: Map::OwnsDescriptors::encode(true);
| Map::Counter::encode(Map::kRetainingCounterStart)

https://codereview.chromium.org/767253002/diff/80001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/767253002/diff/80001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:173: __ cmp(esi, Map::kSlackTrackingCounterEnd + 1);
We should compare against Map::kSlackTrackingCounterEnd here which is the last
value meaning that the slack tracking is still active. Same for other platforms.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5668
src/objects.h:5668: class Counter : public BitField<bool, 28, 4> {};
Please add a comment what is this counter for and please mention that the slack
tracking is considered enabled when the counter is in the range
[kSlackTrackingCounterStart, kSlackTrackingCounterEnd].

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5669
src/objects.h:5669: static const int kSlackTrackingCounterStart = 15;
Counter::kMax

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5678
src/objects.h:5678: (kRetainingCounterEnd >> (Counter::kSize - 1)));
I think we don't need this asserts since we don't have
|done_inobject_slack_tracking| bit anymore and we compare counter values against
ranges explicitly.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode7370
src/objects.h:7370: //   counter in the initial_map and clear the
|is_inobject_slack_tracking_off|
We don't have such a flag.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode7392
src/objects.h:7392: Map::kSlackTrackingCounterStart -
Map::kSlackTrackingCounterEnd;
I think we need + 1 here, since the range in inclusive at both ends.

[ulan, 2014-12-05 14:04:44]

Thanks for review.

https://codereview.chromium.org/767253002/diff/80001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/767253002/diff/80001/src/heap/heap.cc#newcode...
src/heap/heap.cc:2360: Map::OwnsDescriptors::encode(true);
On 2014/12/04 10:11:08, Igor Sheludko wrote:
> | Map::Counter::encode(Map::kRetainingCounterStart)

Done.

https://codereview.chromium.org/767253002/diff/80001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/767253002/diff/80001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:173: __ cmp(esi, Map::kSlackTrackingCounterEnd + 1);
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> We should compare against Map::kSlackTrackingCounterEnd here which is the last
> value meaning that the slack tracking is still active. Same for other
platforms.

Done.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5668
src/objects.h:5668: class Counter : public BitField<bool, 28, 4> {};
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> Please add a comment what is this counter for and please mention that the
slack
> tracking is considered enabled when the counter is in the range
> [kSlackTrackingCounterStart, kSlackTrackingCounterEnd].

Done.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5669
src/objects.h:5669: static const int kSlackTrackingCounterStart = 15;
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> Counter::kMax

Set it 14, so that we don't change the current number of iterations.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode5678
src/objects.h:5678: (kRetainingCounterEnd >> (Counter::kSize - 1)));
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> I think we don't need this asserts since we don't have
> |done_inobject_slack_tracking| bit anymore and we compare counter values
against
> ranges explicitly.

Done.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode7370
src/objects.h:7370: //   counter in the initial_map and clear the
|is_inobject_slack_tracking_off|
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> We don't have such a flag.

Done.

https://codereview.chromium.org/767253002/diff/80001/src/objects.h#newcode7392
src/objects.h:7392: Map::kSlackTrackingCounterStart -
Map::kSlackTrackingCounterEnd;
On 2014/12/04 10:11:09, Igor Sheludko wrote:
> I think we need + 1 here, since the range in inclusive at both ends.

Done.

[Igor Sheludko, 2014-12-05 14:49:09]

lgtm with a nit:

https://codereview.chromium.org/767253002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/767253002/diff/100001/src/objects.cc#newcode1...
src/objects.cc:10317: DCHECK(map->counter() >= Map::kSlackTrackingCounterEnd);
I'm afraid that this assert could fail if we get here from JSConstructStub. I
would say:
  DCHECK(map->counter() >= Map::kSlackTrackingCounterEnd - 1);

[ulan, 2014-12-05 14:52:17]

On 2014/12/05 14:49:09, Igor Sheludko wrote:
> lgtm with a nit:
> 
> https://codereview.chromium.org/767253002/diff/100001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/767253002/diff/100001/src/objects.cc#newcode1...
> src/objects.cc:10317: DCHECK(map->counter() >= Map::kSlackTrackingCounterEnd);
> I'm afraid that this assert could fail if we get here from JSConstructStub. I
> would say:
>   DCHECK(map->counter() >= Map::kSlackTrackingCounterEnd - 1);

Fixed it, thanks.

[ulan, 2014-12-05 14:52:23]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2014-12-05 14:53:03]

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

[commit-bot: I haz the power, 2014-12-05 15:28:16]

Committed patchset #4 (id:120001)

[Vyacheslav Egorov (Google), 2014-12-08 08:51:04]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2014-12-08 08:51:05]

fyi: this regressed EarleyBoyer by 40%

[ulan, 2014-12-08 10:25:47]

On 2014/12/08 08:51:05, Vyacheslav Egorov (Google) wrote:
> fyi: this regressed EarleyBoyer by 40%
Fix landing: https://codereview.chromium.org/779293003/