Age code to allow reclaiming old unexecuted functions

Add code again to allow reclaiming old unexecuted functions.

When code objects in the heap for FUNCTIONs and OPTIMIZED_FUNCTIONs are marked by the GC, their prologue is patched with a call to a stub that removes the patch. This allows the collector to quickly identify code objects that haven't been executed since the last full collection (they are the ones that sill contain the patch). The functionality is currently disabled, but can be activated by specifying the "--age-code". 

R=mstarzinger@chromium.org

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

[danno, 2012-09-20 16:00:27]

Platform ports are not done, but can you please take a first rough look?

[Michael Starzinger, 2012-09-21 09:43:19]

First round of comments.

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc#newcode612
src/compiler.cc:612: result->code()->MakeYoung();
I think we also might want to make the code young again when it get's installed
from the optimized code cache during lazy compilation. The static
InstallCodeFromOptimizedCodeMap() helper in this file should be the right
bottleneck for that.

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc#newcode673
src/compiler.cc:673: }
You should also make the code young here.

https://codereview.chromium.org/10837037/diff/18002/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/heap.h#newcode1549
src/heap.h:1549: MarkingParity marking_parity() {
We shouldn't expose the current parity through the Heap object, because the
parity only makes sense while marking is in progress. Also see my comments in
mark-compact.cc and objects.cc about that.

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
File src/ia32/assembler-ia32-inl.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
src/ia32/assembler-ia32-inl.h:179: Code* RelocInfo::code_age_stub() {
ASSERT(rmode_ == RelocInfo::CODE_AGE_SEQUENCE);

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
src/ia32/assembler-ia32-inl.h:189: void RelocInfo::set_code_age_stub(Code* stub)
{
ASSERT(rmode_ == RelocInfo::CODE_AGE_SEQUENCE);

https://codereview.chromium.org/10837037/diff/18002/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/liveedit.cc#newcode1273
src/liveedit.cc:1273: code->MakeYoung();
Why do we make the code young here?

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:65: marking_parity_(ODD_MARKING_PARITY),
The default should be NO_MARKING_PARITY I think.

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:402: if (marking_parity_ == EVEN_MARKING_PARITY) {
I think it would be safer to set the marking parity in
MarkCompactCollector::Prepare before we start a cycle and then reset it back to
NO_MARKING_PARITY again in MarkCompactCollector::Finish. Thereby we will catch
misuse of the parity value somewhere outside a cycle, where it makes no sense.

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:2719: if (stub != NULL) {
Can the stub really be NULL here? Shouldn't we only visit the relocation info
when there actually is an aging stub call present?

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl....
src/objects-visiting-inl.h:236: if (target != NULL) {
We should only reach this visit function if there actually is a target.

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl....
src/objects-visiting-inl.h:267: code->MakeOlder();
We can pass heap->mark_compact_collector()->marking_parity() here.

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting.h#ne...
src/objects-visiting.h:396: static inline void VisitCodeAgeSequence(Heap* heap,
RelocInfo* rinfo);
Move this up two lines above the empty implementations.

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc#newcode8520
src/objects.cc:8520: void Code::MakeOlder() {
I think MakeOlder should only be called from the GC, which knows the current
parity. So can we pass the parity in as a parameter?

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc#newcode8525
src/objects.cc:8525: MarkingParity current_parity = GetHeap()->marking_parity();
Can we remove the marking_parity getter in Heap?

https://codereview.chromium.org/10837037/diff/18002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects.h#newcode187
src/objects.h:187: enum MarkingParity {
Can we have a short comment on what the marking parity means?

https://codereview.chromium.org/10837037/diff/18002/src/objects.h#newcode8945
src/objects.h:8945: virtual void VisitCodeAgeSequence(RelocInfo* rinfo);
Add a comment of some sort ...

// Visits a code aging site in the code prologue.

[Michael Starzinger, 2012-10-02 14:58:59]

Second round with some follow-up comments, now that I am working on the
incremental code flushing. Also as discussed offline, this CL will require
rebasing after I land one of my CLs.

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc#newco...
src/mark-compact.cc:1205: return function->code()->IsOld() && IsFlushable(heap,
shared_info);
See comment below.

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc#newco...
src/mark-compact.cc:1210: if (!shared_info->code()->IsOld()) return false;
1) We shouldn't remove the check for the marking bit of the code, because it
makes sure that we don't throw away un-optimized code that is required for
bailout from optimized code.

2) Also I don't think it makes much sense to flush code that is strongly
reachable through other paths, that will rather lead to increased memory
consumption because we keep recompiling code without actually collecting it.

3) Also it might be better to move this check further to the bottom, because it
is more expensive than the others I guess. But I don't have a strong opinion
about this last point.

[danno, 2012-10-25 10:07:23]

Please take another look, I think this is ready to go on ia32, x64 and ARM.

Please pay close attention to the logic in IsFlushable() and the order in which
code aging happens with respect to code body visitation.

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc#newcode612
src/compiler.cc:612: result->code()->MakeYoung();
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> I think we also might want to make the code young again when it get's
installed
> from the optimized code cache during lazy compilation. The static
> InstallCodeFromOptimizedCodeMap() helper in this file should be the right
> bottleneck for that.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/compiler.cc#newcode673
src/compiler.cc:673: }
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> You should also make the code young here.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/heap.h#newcode1549
src/heap.h:1549: MarkingParity marking_parity() {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> We shouldn't expose the current parity through the Heap object, because the
> parity only makes sense while marking is in progress. Also see my comments in
> mark-compact.cc and objects.cc about that.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
File src/ia32/assembler-ia32-inl.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
src/ia32/assembler-ia32-inl.h:179: Code* RelocInfo::code_age_stub() {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> ASSERT(rmode_ == RelocInfo::CODE_AGE_SEQUENCE);

Done.

https://codereview.chromium.org/10837037/diff/18002/src/ia32/assembler-ia32-i...
src/ia32/assembler-ia32-inl.h:189: void RelocInfo::set_code_age_stub(Code* stub)
{
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> ASSERT(rmode_ == RelocInfo::CODE_AGE_SEQUENCE);

Done.

https://codereview.chromium.org/10837037/diff/18002/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/liveedit.cc#newcode1273
src/liveedit.cc:1273: code->MakeYoung();
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Why do we make the code young here?

Done.

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:65: marking_parity_(ODD_MARKING_PARITY),
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> The default should be NO_MARKING_PARITY I think.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:402: if (marking_parity_ == EVEN_MARKING_PARITY) {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> I think it would be safer to set the marking parity in
> MarkCompactCollector::Prepare before we start a cycle and then reset it back
to
> NO_MARKING_PARITY again in MarkCompactCollector::Finish. Thereby we will catch
> misuse of the parity value somewhere outside a cycle, where it makes no sense.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/mark-compact.cc#newco...
src/mark-compact.cc:2719: if (stub != NULL) {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Can the stub really be NULL here? Shouldn't we only visit the relocation info
> when there actually is an aging stub call present?

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl....
src/objects-visiting-inl.h:236: if (target != NULL) {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> We should only reach this visit function if there actually is a target.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting-inl....
src/objects-visiting-inl.h:267: code->MakeOlder();
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> We can pass heap->mark_compact_collector()->marking_parity() here.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects-visiting.h#ne...
src/objects-visiting.h:396: static inline void VisitCodeAgeSequence(Heap* heap,
RelocInfo* rinfo);
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Move this up two lines above the empty implementations.

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc#newcode8520
src/objects.cc:8520: void Code::MakeOlder() {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> I think MakeOlder should only be called from the GC, which knows the current
> parity. So can we pass the parity in as a parameter?

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects.cc#newcode8525
src/objects.cc:8525: MarkingParity current_parity = GetHeap()->marking_parity();
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Can we remove the marking_parity getter in Heap?

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/10837037/diff/18002/src/objects.h#newcode187
src/objects.h:187: enum MarkingParity {
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Can we have a short comment on what the marking parity means?

Done.

https://codereview.chromium.org/10837037/diff/18002/src/objects.h#newcode8945
src/objects.h:8945: virtual void VisitCodeAgeSequence(RelocInfo* rinfo);
On 2012/09/21 09:43:19, Michael Starzinger wrote:
> Add a comment of some sort ...
> 
> // Visits a code aging site in the code prologue.

Done.

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc#newco...
src/mark-compact.cc:1205: return function->code()->IsOld() && IsFlushable(heap,
shared_info);
On 2012/10/02 14:58:59, Michael Starzinger wrote:
> See comment below.

Done.

https://codereview.chromium.org/10837037/diff/16023/src/mark-compact.cc#newco...
src/mark-compact.cc:1210: if (!shared_info->code()->IsOld()) return false;
On 2012/10/02 14:58:59, Michael Starzinger wrote:
> 1) We shouldn't remove the check for the marking bit of the code, because it
> makes sure that we don't throw away un-optimized code that is required for
> bailout from optimized code.
> 
> 2) Also I don't think it makes much sense to flush code that is strongly
> reachable through other paths, that will rather lead to increased memory
> consumption because we keep recompiling code without actually collecting it.
> 
> 3) Also it might be better to move this check further to the bottom, because
it
> is more expensive than the others I guess. But I don't have a strong opinion
> about this last point.

Done.

[Michael Starzinger, 2012-11-02 15:29:34]

LGTM (with a final round of comments). The most important one is the one about
flushing optimized code.

http://codereview.chromium.org/10837037/diff/40001/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/10837037/diff/40001/src/mark-compact.cc#newcod...
src/mark-compact.cc:2360: rinfo->set_code_age_stub(Code::cast(stub));
Can we use the "if (target != old_target)" pattern here as well? Thereby we
avoid flushing the instruction cache when the stub doesn't actually change.

http://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

http://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:291: if (FLAG_age_code && !Serializer::enabled()) {
Technically it shouldn't make any difference in this case, but I would sleep
better if we could do the mutation before we scan the code body. Can we move
this block to before we call CodeIterateBody() here?

http://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:480: return function->code()->IsOld() &&
IsFlushable(heap, shared_info);
We cannot yet flush optimized code because they are not recorded as candidates.
I will be working on that next. For now we need to leave in the original check
in place.

Also, can we do the age-check as a separate step behind the FLAG_age_code flag
and in the end always call through to IsFlushable() for the shared function? I
think that's easier to read.

// Check age of optimized code.
if (FLAG_age_code && !function->code()->IsOld()) {
  return false;
}

return IsFlushable(heap, shared_info);

[danno, 2012-11-09 13:05:30]

Feedback addressed, landed.

https://codereview.chromium.org/10837037/diff/40001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/10837037/diff/40001/src/mark-compact.cc#newco...
src/mark-compact.cc:2360: rinfo->set_code_age_stub(Code::cast(stub));
On 2012/11/02 15:29:34, Michael Starzinger wrote:
> Can we use the "if (target != old_target)" pattern here as well? Thereby we
> avoid flushing the instruction cache when the stub doesn't actually change.

Done.

https://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl....
src/objects-visiting-inl.h:291: if (FLAG_age_code && !Serializer::enabled()) {
On 2012/11/02 15:29:34, Michael Starzinger wrote:
> Technically it shouldn't make any difference in this case, but I would sleep
> better if we could do the mutation before we scan the code body. Can we move
> this block to before we call CodeIterateBody() here?

Done.

https://codereview.chromium.org/10837037/diff/40001/src/objects-visiting-inl....
src/objects-visiting-inl.h:480: return function->code()->IsOld() &&
IsFlushable(heap, shared_info);
On 2012/11/02 15:29:34, Michael Starzinger wrote:
> We cannot yet flush optimized code because they are not recorded as
candidates.
> I will be working on that next. For now we need to leave in the original check
> in place.
> 
> Also, can we do the age-check as a separate step behind the FLAG_age_code flag
> and in the end always call through to IsFlushable() for the shared function? I
> think that's easier to read.
> 
> // Check age of optimized code.
> if (FLAG_age_code && !function->code()->IsOld()) {
>   return false;
> }
> 
> return IsFlushable(heap, shared_info);

Done.