Fix memory leak caused by treating Code::next_code_link as strong in marker.

Fix memory leak caused by treating Code::next_code_link as strong in marker.

BUG=
TEST=test/cctest/NextCodeLinkIsWeak
R=mstarzinger@chromium.org

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

[ulan, 2014-03-01 13:07:14]

PTAL, discovered this leak while investigating an unrelated crash.

WDYT about introducing weak_pointer_mode to visitors? If this is OK, then I can
move other weak pointers under this check into the base visitor. This would make
explicit which fields are weak and which are not.

Aliasing next_code_link to type_feedback_info is fragile. Maybe I should rename
both fields to reflect aliasing or just add unalias them by adding another field
to Code?

[Michael Starzinger, 2014-03-04 12:31:11]

Nice catch. Just two suggestions.

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

https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
src/objects-visiting-inl.h:936: !is_optimized_code()) {
As discussed offline: AFAICT, currently we can afford to not have the fields
overlap as the padding is big enough on 32bit and 64bit architectures. So
"un-overlapping" them woukd definitely make things easier to read.

https://codereview.chromium.org/181833004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/181833004/diff/20001/src/objects.h#newcode10690
src/objects.h:10690: enum WeakPointerMode {
As discussed offline: I am not a particular fan of having this mode, because it
assumes that all visitors either visit _all_ or _none_ weak pointers and I am
not sure this hold in general.

Alternatively I would suggest having a special visitor method (e.g.
VisitCodeNextPointer) that the concrete visitor can override to handle this slot
specially. This could be applied to both, the virtual as well as the static
visitor I think.

[titzer, 2014-03-05 08:58:02]

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

https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
src/objects-visiting-inl.h:936: !is_optimized_code()) {
On 2014/03/04 12:31:12, Michael Starzinger wrote:
> As discussed offline: AFAICT, currently we can afford to not have the fields
> overlap as the padding is big enough on 32bit and 64bit architectures. So
> "un-overlapping" them woukd definitely make things easier to read.

+1 unoverlap. I am sorry for overlapping them in the first place.

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

https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3719: TEST(NextCodeLinkIsWeak) {
Why not make a test case that creates optimized code objects and links them
together directly? This test case seems pretty brittle and makes lots of
assumptions about how the compiler links functions and code objects together.

[ulan, 2014-03-06 11:13:32]

Un-overlapped and added VisitCodeNextPointer, PTAL.

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

https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
src/objects-visiting-inl.h:936: !is_optimized_code()) {
On 2014/03/05 08:58:02, titzer wrote:
> On 2014/03/04 12:31:12, Michael Starzinger wrote:
> > As discussed offline: AFAICT, currently we can afford to not have the fields
> > overlap as the padding is big enough on 32bit and 64bit architectures. So
> > "un-overlapping" them woukd definitely make things easier to read.
> 
> +1 unoverlap. I am sorry for overlapping them in the first place.
Nope, it is my fault. I remember introducing this field and overlapping it.
Sorry for that.

https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
src/objects-visiting-inl.h:936: !is_optimized_code()) {
On 2014/03/04 12:31:12, Michael Starzinger wrote:
> As discussed offline: AFAICT, currently we can afford to not have the fields
> overlap as the padding is big enough on 32bit and 64bit architectures. So
> "un-overlapping" them woukd definitely make things easier to read.

Done.

https://codereview.chromium.org/181833004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/181833004/diff/20001/src/objects.h#newcode10690
src/objects.h:10690: enum WeakPointerMode {
On 2014/03/04 12:31:12, Michael Starzinger wrote:
> As discussed offline: I am not a particular fan of having this mode, because
it
> assumes that all visitors either visit _all_ or _none_ weak pointers and I am
> not sure this hold in general.
> 
> Alternatively I would suggest having a special visitor method (e.g.
> VisitCodeNextPointer) that the concrete visitor can override to handle this
slot
> specially. This could be applied to both, the virtual as well as the static
> visitor I think.

Done. Thanks for suggestion, it is cleaner indeed.

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

https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3719: TEST(NextCodeLinkIsWeak) {
It assumes two things:
- next_code_link is used for linking
- the optimized code list is a stack (the last optimized code object points to
the previously optimized code)

Alternative to this would be to manage the list manually. Then I would have to
- optimize the two functions
- remove the optimized code objects from the context[OPTIMIZED_CODE_LIST] list
- add two code objects to the list manually.

I agree that this is less brittle in the sense that it wouldn't break if the
real list management changes. But then there is a risk that the list management
in this test and the real list management diverge and the test no longer tests
the implementation.

[titzer, 2014-03-06 11:29:33]

On 2014/03/06 11:13:32, ulan wrote:
> Un-overlapped and added VisitCodeNextPointer, PTAL.
> 
>
https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl.h
> File src/objects-visiting-inl.h (right):
> 
>
https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
> src/objects-visiting-inl.h:936: !is_optimized_code()) {
> On 2014/03/05 08:58:02, titzer wrote:
> > On 2014/03/04 12:31:12, Michael Starzinger wrote:
> > > As discussed offline: AFAICT, currently we can afford to not have the
fields
> > > overlap as the padding is big enough on 32bit and 64bit architectures. So
> > > "un-overlapping" them woukd definitely make things easier to read.
> > 
> > +1 unoverlap. I am sorry for overlapping them in the first place.
> Nope, it is my fault. I remember introducing this field and overlapping it.
> Sorry for that.
> 
>
https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl...
> src/objects-visiting-inl.h:936: !is_optimized_code()) {
> On 2014/03/04 12:31:12, Michael Starzinger wrote:
> > As discussed offline: AFAICT, currently we can afford to not have the fields
> > overlap as the padding is big enough on 32bit and 64bit architectures. So
> > "un-overlapping" them woukd definitely make things easier to read.
> 
> Done.
> 
> https://codereview.chromium.org/181833004/diff/20001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/181833004/diff/20001/src/objects.h#newcode10690
> src/objects.h:10690: enum WeakPointerMode {
> On 2014/03/04 12:31:12, Michael Starzinger wrote:
> > As discussed offline: I am not a particular fan of having this mode, because
> it
> > assumes that all visitors either visit _all_ or _none_ weak pointers and I
am
> > not sure this hold in general.
> > 
> > Alternatively I would suggest having a special visitor method (e.g.
> > VisitCodeNextPointer) that the concrete visitor can override to handle this
> slot
> > specially. This could be applied to both, the virtual as well as the static
> > visitor I think.
> 
> Done. Thanks for suggestion, it is cleaner indeed.
> 
> https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc
> File test/cctest/test-heap.cc (right):
> 
>
https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc...
> test/cctest/test-heap.cc:3719: TEST(NextCodeLinkIsWeak) {
> It assumes two things:
> - next_code_link is used for linking
> - the optimized code list is a stack (the last optimized code object points to
> the previously optimized code)
> 
> Alternative to this would be to manage the list manually. Then I would have to
> - optimize the two functions
> - remove the optimized code objects from the context[OPTIMIZED_CODE_LIST] list
> - add two code objects to the list manually.
> 
> I agree that this is less brittle in the sense that it wouldn't break if the
> real list management changes. But then there is a risk that the list
management
> in this test and the real list management diverge and the test no longer tests
> the implementation.

Agree with your above statement. But it's easy just to have two tests.

[ulan, 2014-03-06 12:19:30]

> Agree with your above statement. But it's easy just to have two tests.
Done.

[Michael Starzinger, 2014-03-06 13:43:51]

LGTM with one comment.

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

https://codereview.chromium.org/181833004/diff/120001/src/mark-compact.cc#new...
src/mark-compact.cc:1864: void VisitNextCodeLink(Object** p) { }
This should actually be unreachable, because there is no "NextCodeLink" in the
root-set. There might be code objects in the root-set, but their body will be
visited using the static visitor (see line 1882) below. Can we add an
UNREACHABLE() assertion instead?

[ulan, 2014-03-13 14:06:51]

Thanks for review, landing.

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

https://codereview.chromium.org/181833004/diff/120001/src/mark-compact.cc#new...
src/mark-compact.cc:1864: void VisitNextCodeLink(Object** p) { }
On 2014/03/06 13:43:52, Michael Starzinger wrote:
> This should actually be unreachable, because there is no "NextCodeLink" in the
> root-set. There might be code objects in the root-set, but their body will be
> visited using the static visitor (see line 1882) below. Can we add an
> UNREACHABLE() assertion instead?

As discussed offline, this is called in ProcessTopOptimizedFrame. Added a
comment.

[ulan, 2014-03-13 14:09:27]

Committed patchset #7 manually as r19897 (presubmit successful).