Tenure allocation sites only when semi-space is maximum size.

Tenure allocation sites only when semi-space is maximum size.

BUG=
R=mvstanton@chromium.org

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

[mvstanton, 2014-06-02 13:31:24]

Hi Hannes, it is great to see the maybe-tenure state incorporated. Just two
related comments,
--Michael

https://codereview.chromium.org/309623007/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/309623007/diff/1/src/heap.cc#newcode1304
src/heap.cc:1304: changed_to_max_semi_space_size_ = true;
Is the boolean really needed? Why not just ask new_space_.IsAtMaximumCapacity().
I'm thinking it might be some redundant/duplicate state.

https://codereview.chromium.org/309623007/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/309623007/diff/1/src/objects-inl.h#newcode1586
src/objects-inl.h:1586: if (heap->changed_to_max_semi_space_size() &&
Since the allocation sites are processed in bulk, and this predicate
(changed_to_max_semi_space_size()) won't change between the start of the
processing and the end of it, it might be better to pass that value in to
DigestPretenuringFeedback().

What do you think? This might also reduce the need for a boolean versus a
predicate that just checks the space capacity against max capacity.

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

https://codereview.chromium.org/309623007/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:2198: i::FLAG_min_semi_space_size = 8;
Why is this being done? Probably worth a comment.

[Hannes Payer (out of office), 2014-06-02 14:56:52]

https://codereview.chromium.org/309623007/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/309623007/diff/1/src/heap.cc#newcode1304
src/heap.cc:1304: changed_to_max_semi_space_size_ = true;
On 2014/06/02 13:31:25, mvstanton wrote:
> Is the boolean really needed? Why not just ask
new_space_.IsAtMaximumCapacity().
> I'm thinking it might be some redundant/duplicate state.

As discussed offline, we need a separate flag to identify valid pretenuring
feedback

https://codereview.chromium.org/309623007/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/309623007/diff/1/src/objects-inl.h#newcode1586
src/objects-inl.h:1586: if (heap->changed_to_max_semi_space_size() &&
On 2014/06/02 13:31:25, mvstanton wrote:
> Since the allocation sites are processed in bulk, and this predicate
> (changed_to_max_semi_space_size()) won't change between the start of the
> processing and the end of it, it might be better to pass that value in to
> DigestPretenuringFeedback().
> 
> What do you think? This might also reduce the need for a boolean versus a
> predicate that just checks the space capacity against max capacity.

Done.

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

https://codereview.chromium.org/309623007/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:2198: i::FLAG_min_semi_space_size = 8;
On 2014/06/02 13:31:25, mvstanton wrote:
> Why is this being done? Probably worth a comment.

Done.

[mvstanton, 2014-06-03 05:44:54]

Great, lgtm. Just one nit: the name "valid_feedback" is a little confusing - I
wonder how you can be allowed to do *anything* if a variable with that name is
false. :D Maybe that name gets a little less abstract, back towards "maximum
capacity"?

[Hannes Payer (out of office), 2014-06-03 07:35:59]

Done.

[Hannes Payer (out of office), 2014-06-03 07:55:54]

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