Inobject slack tracking is done on a per-closure basis instead of per-shared info basis.

Inobject slack tracking is done on a per-closure basis instead of per-shared info basis.
This fixes inobject slack tracking for prototype inheritance pattern that uses closures.

R=mstarzinger@chromium.org

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

[Igor Sheludko, 2014-05-16 15:53:20]

PTAL. Currently only for ia32, the other platforms are on the way.

[Igor Sheludko, 2014-05-16 19:09:28]

Other archs are ready.

[Igor Sheludko, 2014-05-19 16:47:52]

Bugfixes, improvements and cleanup in constructor stubs.

[Michael Starzinger, 2014-05-21 10:42:00]

I like this change. Mostly nits, only one major comment in the JSConstructStub.
Note that I only looked at the ia32 port, but my comments probably apply to all
ports.

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

https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:161: // esi: slack tracking counter
nit: At this point esi does not yet contain the counter, it is loaded below.
Let's drop this comment.

https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:242: __ jmp(&done_field_initialization);
This will skip initialization of the memento even though we allocated space for
it. This will probably throw off the heap verifier.

Talking to Michael Stanton, he agreed that the safest path forward would be to
not allocate a Memento while slack-tracking is in progress.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h
File src/objects.h (left):

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#oldcode7008
src/objects.h:7008: inline void BeforeVisitingPointers();
The only reason we needed this callback was for detaching the initial map from
the SharedFunctionInfo. Since we ne longer do that I propose to remove this
callback, this in turn makes the BeforeVisitingSharedFunctionInfo function in
the static visitors obsolete as well.

I am totally fine to do that in a follow-up CL, because this one is already
complex enough.

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

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5967
src/objects.h:5967: class DoneInobjectSlackTracking:  public BitField<bool, 28,
1> {};
nit: Alignment looks off.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5970
src/objects.h:5970: class ConstructionCount:          public BitField<unsigned,
29, 3> {};
nit: With three bits it should be fine to use plain "int" here.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7511
src/objects.h:7511: static const unsigned kGenerousAllocationCount =
nit: Likewise, plain "int" should do it here.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7512
src/objects.h:7512: (1 << Map::ConstructionCount::kSize) - 1;
nit: Just use "Map::ConstructionCount::kMax" here.

https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-memento...
File test/cctest/test-mementos.cc (right):

https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-memento...
test/cctest/test-mementos.cc:94: unsigned int call_count =
JSFunction::kGenerousAllocationCount + 2;
nit: Likewise, plain "int" should do it here.

[Igor Sheludko, 2014-05-22 08:05:42]

Addressed comments for all platforms.

I did a quick check with both enabled and disabled "pretenuring call new" modes.
It seems to work.

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

https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:161: // esi: slack tracking counter
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: At this point esi does not yet contain the counter, it is loaded below.
> Let's drop this comment.

Done.

https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:242: __ jmp(&done_field_initialization);
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> This will skip initialization of the memento even though we allocated space
for
> it. This will probably throw off the heap verifier.
> 
> Talking to Michael Stanton, he agreed that the safest path forward would be to
> not allocate a Memento while slack-tracking is in progress.

Done. Now mementos are always initialized if they are allocated.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h
File src/objects.h (left):

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#oldcode7008
src/objects.h:7008: inline void BeforeVisitingPointers();
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> The only reason we needed this callback was for detaching the initial map from
> the SharedFunctionInfo. Since we ne longer do that I propose to remove this
> callback, this in turn makes the BeforeVisitingSharedFunctionInfo function in
> the static visitors obsolete as well.
> 
> I am totally fine to do that in a follow-up CL, because this one is already
> complex enough.

I'll do the cleanup in a next CL.

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

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5967
src/objects.h:5967: class DoneInobjectSlackTracking:  public BitField<bool, 28,
1> {};
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: Alignment looks off.

Done.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5970
src/objects.h:5970: class ConstructionCount:          public BitField<unsigned,
29, 3> {};
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: With three bits it should be fine to use plain "int" here.

Done.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7511
src/objects.h:7511: static const unsigned kGenerousAllocationCount =
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: Likewise, plain "int" should do it here.

Done.

https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7512
src/objects.h:7512: (1 << Map::ConstructionCount::kSize) - 1;
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: Just use "Map::ConstructionCount::kMax" here.

Done.

https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-memento...
File test/cctest/test-mementos.cc (right):

https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-memento...
test/cctest/test-mementos.cc:94: unsigned int call_count =
JSFunction::kGenerousAllocationCount + 2;
On 2014/05/21 10:42:01, Michael Starzinger wrote:
> nit: Likewise, plain "int" should do it here.

Done. Actually now there should be exactly call_count mementos allocated.

[Michael Starzinger, 2014-05-22 10:34:42]

LGTM. I like this.

[Igor Sheludko, 2014-05-22 12:35:59]

Committed patchset #4 manually as r21436 (presubmit successful).