Allocation site pretenuring.

Allocation site pretenuring.

Pretenuring decisions are made based on allocation site lifetime statistics.

BUG=
R=mstarzinger@chromium.org, mvstanton@chromium.org

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

[mvstanton, 2013-11-30 12:20:40]

A couple comments. Very cool stuff.
--Michael

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc#newcode1941
src/heap.cc:1941: casted->dependent_code()->DeoptimizeDependentCodeGroup(
This is a good thing, but don't you also need to reset the pretenure_decision
field in the allocation site? I think right now it won't know that the decision
needs to be revisited.

In fact, maybe there should be a method in AllocationSite that does these two
things, like ResetPretenureDecision(), which would naturally enough deopt any
code that depends on the previous value of that decision.

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc#newcode1949
src/heap.cc:1949: void Heap::EvaluateLocalPretenuring(int64_t objects_before_gc)
{
objects_before_gc: Can you get "sizeof" in the name of this variable? I'm
confused about what it is...# of objects, total byte size of them, etc.

[mvstanton, 2013-12-02 10:14:45]

lgtm

[Hannes Payer (out of office), 2013-12-02 11:27:15]

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc#newcode1941
src/heap.cc:1941: casted->dependent_code()->DeoptimizeDependentCodeGroup(
On 2013/11/30 12:20:41, mvstanton wrote:
> This is a good thing, but don't you also need to reset the pretenure_decision
> field in the allocation site? I think right now it won't know that the
decision
> needs to be revisited.
> 
> In fact, maybe there should be a method in AllocationSite that does these two
> things, like ResetPretenureDecision(), which would naturally enough deopt any
> code that depends on the previous value of that decision.

Done.

https://codereview.chromium.org/96783002/diff/50001/src/heap.cc#newcode1949
src/heap.cc:1949: void Heap::EvaluateLocalPretenuring(int64_t objects_before_gc)
{
On 2013/11/30 12:20:41, mvstanton wrote:
> objects_before_gc: Can you get "sizeof" in the name of this variable? I'm
> confused about what it is...# of objects, total byte size of them, etc.

Done.

[Hannes Payer (out of office), 2013-12-02 11:47:11]

This CL should also add the adapted tests since we turn on the allocation site
pretenuring mode partially. I will do that and upload them into this CL.

[Michael Starzinger, 2013-12-02 15:12:54]

One round of comments. Looking good.

https://codereview.chromium.org/96783002/diff/110001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/110001/src/heap.cc#newcode520
src/heap.cc:520: casted->dependent_code()->DeoptimizeDependentCodeGroup(
IMHO this should be done as part of DigestPretenuringFeedback() instead of here.
Also we need to deopt whenever the decision changes in either way, not just when
we switch from NOT_TENURED to TENURED I think.

https://codereview.chromium.org/96783002/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/96783002/diff/110001/src/hydrogen.cc#newcode9349
src/hydrogen.cc:9349: site_context->current()->AddDependentCompilationInfo(
IMHO both accesses here should be behind the FLAG_allocation_site_pretenuring
flag. If that flag is false, we should either fall-back to the global mode or to
NOT_TENURED I guess. WDYT?

Unless you are planning to remove that flag completely soon, then we could just
start dropping checks already.

[Hannes Payer (out of office), 2014-01-09 14:37:12]

https://codereview.chromium.org/96783002/diff/110001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/110001/src/heap.cc#newcode520
src/heap.cc:520: casted->dependent_code()->DeoptimizeDependentCodeGroup(
On 2013/12/02 15:12:54, Michael Starzinger wrote:
> IMHO this should be done as part of DigestPretenuringFeedback() instead of
here.
> Also we need to deopt whenever the decision changes in either way, not just
when
> we switch from NOT_TENURED to TENURED I think.

Done.

https://codereview.chromium.org/96783002/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/96783002/diff/110001/src/hydrogen.cc#newcode9349
src/hydrogen.cc:9349: site_context->current()->AddDependentCompilationInfo(
On 2013/12/02 15:12:54, Michael Starzinger wrote:
> IMHO both accesses here should be behind the FLAG_allocation_site_pretenuring
> flag. If that flag is false, we should either fall-back to the global mode or
to
> NOT_TENURED I guess. WDYT?
> 
> Unless you are planning to remove that flag completely soon, then we could
just
> start dropping checks already.

Done. Makes sense to have both flags for some time.

[mvstanton, 2014-01-09 15:33:29]

I have a couple of nits, but this update still LGTM.

https://codereview.chromium.org/96783002/diff/240001/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/96783002/diff/240001/src/heap-inl.h#newcode491
src/heap-inl.h:491: AllocationMemento* memento =
AllocationMemento::FindForHeapObject(
nit: you could pass the heap into FindForHeapObject now that you are looking it
up once.

https://codereview.chromium.org/96783002/diff/240001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/240001/src/heap.cc#newcode1991
src/heap.cc:1991: void Heap::ResetAllAllocationSitesDependentCode() {
nit: ResetAllTenuredAllocationSitesDependentCode() and
ResetAllAllocationSitesDependentCode() could be the same function, accepting an
enum of ALL or TENURED.

[Hannes Payer (out of office), 2014-01-09 16:01:08]

https://codereview.chromium.org/96783002/diff/240001/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/96783002/diff/240001/src/heap-inl.h#newcode491
src/heap-inl.h:491: AllocationMemento* memento =
AllocationMemento::FindForHeapObject(
On 2014/01/09 15:33:30, mvstanton wrote:
> nit: you could pass the heap into FindForHeapObject now that you are looking
it
> up once.

Done.

https://codereview.chromium.org/96783002/diff/240001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/96783002/diff/240001/src/heap.cc#newcode1991
src/heap.cc:1991: void Heap::ResetAllAllocationSitesDependentCode() {
On 2014/01/09 15:33:30, mvstanton wrote:
> nit: ResetAllTenuredAllocationSitesDependentCode() and
> ResetAllAllocationSitesDependentCode() could be the same function, accepting
an
> enum of ALL or TENURED.

Done. Thanks for this comment. ALL is actually not needed. When we switch on
high promotion mode, we should deopt all all code with not-tenured allocation
sites.

[Michael Starzinger, 2014-01-10 09:10:23]

LGTM.

https://codereview.chromium.org/96783002/diff/300001/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/96783002/diff/300001/src/heap-inl.h#newcode487
src/heap-inl.h:487: Heap* heap = object->GetIsolate()->heap();
nit: Just "object->GetHeap()" here.

https://codereview.chromium.org/96783002/diff/300001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/96783002/diff/300001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:2211: "  for (var i = 0; i < 100000-1; i++) {"
I hope the runtime of these tests didn't explode with this change. Could make
sense to double-check that.

[Hannes Payer (out of office), 2014-01-10 12:03:30]

https://codereview.chromium.org/96783002/diff/300001/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/96783002/diff/300001/src/heap-inl.h#newcode487
src/heap-inl.h:487: Heap* heap = object->GetIsolate()->heap();
On 2014/01/10 09:10:23, Michael Starzinger wrote:
> nit: Just "object->GetHeap()" here.

Done.

https://codereview.chromium.org/96783002/diff/300001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/96783002/diff/300001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:2211: "  for (var i = 0; i < 100000-1; i++) {"
On 2014/01/10 09:10:23, Michael Starzinger wrote:
> I hope the runtime of these tests didn't explode with this change. Could make
> sense to double-check that.

Thanks for the comment, I was able to reduce the number of allocated objects by
limiting the maximum new space size.

[Hannes Payer (out of office), 2014-01-10 12:12:04]

Committed patchset #12 manually as r18532 (presubmit successful).