Fixes a couple problems with GC of unoptimized code.

Fixes a couple problems with GC of unoptimized code.

1. Instead of making a pass before the Marking phase,
   this change does not visit code pointers in functions
   during marking. Then after marking, if the code has
   still not been marked, code pointers in functions
   are nulled out.
2. Since code pointers in functions may be nulled out,
   functions are no longer used as proxies for code in
   deoptimization info.

R=iposva@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30600

[zra, 2013-11-15 18:29:16]

Here are the changes we talked about on Thursday morning.

[Ivan Posva, 2013-11-16 00:06:21]

First round of comments.

-Ivan

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1349: if (!function.HasCode()) {
Code should not be detachable as it is live on the stack.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
runtime/vm/deopt_instructions.cc:799: if (!function.HasCode()) {
Should be an assert. The unoptimized code cannot be detached while there is a
reference.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:491: const Code& unoptimized_code =
We should probably keep the code in a local handle until we are certain that we
are going to inline and then we can stuff the unoptimized code into the zone
handle just before returning true.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:244: String name;
This needs about 10 lines of comments why this is safe here.

[zra, 2013-11-18 18:54:31]

+srdjan,fschneider,kmillikn

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1349: if (!function.HasCode()) {
On 2013/11/16 00:06:21, Ivan Posva wrote:
> Code should not be detachable as it is live on the stack.

Changed to ASSERT.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
runtime/vm/deopt_instructions.cc:799: if (!function.HasCode()) {
On 2013/11/16 00:06:21, Ivan Posva wrote:
> Should be an assert. The unoptimized code cannot be detached while there is a
> reference.

Changed to ASSERT.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:491: const Code& unoptimized_code =
On 2013/11/16 00:06:21, Ivan Posva wrote:
> We should probably keep the code in a local handle until we are certain that
we
> are going to inline and then we can stuff the unoptimized code into the zone
> handle just before returning true.

Done.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:244: String name;
On 2013/11/16 00:06:21, Ivan Posva wrote:
> This needs about 10 lines of comments why this is safe here.

Done.

[zra, 2013-11-18 18:54:32]

+srdjan,fschneider,kmillikn

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1349: if (!function.HasCode()) {
On 2013/11/16 00:06:21, Ivan Posva wrote:
> Code should not be detachable as it is live on the stack.

Changed to ASSERT.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
runtime/vm/deopt_instructions.cc:799: if (!function.HasCode()) {
On 2013/11/16 00:06:21, Ivan Posva wrote:
> Should be an assert. The unoptimized code cannot be detached while there is a
> reference.

Changed to ASSERT.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:491: const Code& unoptimized_code =
On 2013/11/16 00:06:21, Ivan Posva wrote:
> We should probably keep the code in a local handle until we are certain that
we
> are going to inline and then we can stuff the unoptimized code into the zone
> handle just before returning true.

Done.

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:244: String name;
On 2013/11/16 00:06:21, Ivan Posva wrote:
> This needs about 10 lines of comments why this is safe here.

Done.

[zra, 2013-11-18 18:56:07]

On 2013/11/18 18:54:32, zra wrote:
> +srdjan,fschneider,kmillikn
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
> File runtime/vm/code_generator.cc (right):
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/code_generato...
> runtime/vm/code_generator.cc:1349: if (!function.HasCode()) {
> On 2013/11/16 00:06:21, Ivan Posva wrote:
> > Code should not be detachable as it is live on the stack.
> 
> Changed to ASSERT.
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
> File runtime/vm/deopt_instructions.cc (right):
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/deopt_instruc...
> runtime/vm/deopt_instructions.cc:799: if (!function.HasCode()) {
> On 2013/11/16 00:06:21, Ivan Posva wrote:
> > Should be an assert. The unoptimized code cannot be detached while there is
a
> > reference.
> 
> Changed to ASSERT.
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
> File runtime/vm/flow_graph_inliner.cc (right):
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/flow_graph_in...
> runtime/vm/flow_graph_inliner.cc:491: const Code& unoptimized_code =
> On 2013/11/16 00:06:21, Ivan Posva wrote:
> > We should probably keep the code in a local handle until we are certain that
> we
> > are going to inline and then we can stuff the unoptimized code into the zone
> > handle just before returning true.
> 
> Done.
> 
> https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc
> File runtime/vm/gc_marker.cc (right):
> 
>
https://codereview.chromium.org/70183010/diff/220001/runtime/vm/gc_marker.cc#...
> runtime/vm/gc_marker.cc:244: String name;
> On 2013/11/16 00:06:21, Ivan Posva wrote:
> > This needs about 10 lines of comments why this is safe here.
> 
> Done.

Should have noted: A significant side-effect of this change is that Function
objects can no longer be used as proxies for their related unoptimized Code
objects.

[srdjan, 2013-11-19 19:18:28]

LGTM with comments

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/block_schedul...
File runtime/vm/block_scheduler.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/block_schedul...
runtime/vm/block_scheduler.cc:56:
flow_graph()->parsed_function().function().unoptimized_code());
Maybe cache unoptimized code in class ParsedFunction when creating it instead of
using function as the proxy?

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1346: const Code& code =
Code::ZoneHandle(frame->LookupDartCode());
Optional, as it will fail in next line: ASSERT(!code.IsNull())

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1349: ASSERT(function.HasCode());
Why does this need to be true? Is this because once optimizations kick in, we
cannot remove code? Please add brief comment, that would also cover the
ASSERT(!original_code.IsNull()).

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1444:
caller_code.GetStaticCallTargetFunctionAt(frame->pc()));
GetStaticCallTargetCodeAt ->target_code -> function

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:552: // IsInlineable above checked HasCode.
Creating a ZoneHandle for the code
s/ZoneHandle/Handle/ since you stored it into a scoped handle not a zone handle.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:146: bool visit_function_code() { return
visit_function_code_; }
const

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:265: bool visit_function_code_;
const

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:476: bool visit_function_code = !try_collecting_code;
const bool

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:6884: const Function& function_;
Do we need function, should we use code_ to return function instead?

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/visitor.h
File runtime/vm/visitor.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/visitor.h#new...
runtime/vm/visitor.h:28: virtual bool visit_function_code() { return true; }
const

[Ivan Posva, 2013-11-19 19:24:48]

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:170: TryDetachingCode();
DetachCode()

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:238: // If the code wasn't marked even after skipping
the function's code
// If the code wasn't strongly visited by other references, then we...

You can also leave the "It will be swept." out as that is a given for Mark-Sweep
and strictly speeking depends on the GC mechanism.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:475: bool try_collecting_code) {
The "try" is unnecessary here. Either you collect code or you don't.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/object.h#newc...
runtime/vm/object.h:1993: friend class RawFunction;
What is this needed for?

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:431: bool try_collecting_code = FLAG_collect_code &&
ShouldTryCollectingCode();
collect_code and ShouldCollectCode() please.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc
File runtime/vm/raw_object.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc...
runtime/vm/raw_object.cc:382: if (fn.HasCode() &&  // Not already detached.
Wouldn't code just be null in this case?

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc...
runtime/vm/raw_object.cc:385: (fn.usage_counter() > 0)) {
Is this still necessary? If it is please comment why exactly it is needed.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:614: friend class MarkingVisitor;
Why is this needed?

[zra, 2013-11-22 17:18:54]

PTAL

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/block_schedul...
File runtime/vm/block_scheduler.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/block_schedul...
runtime/vm/block_scheduler.cc:56:
flow_graph()->parsed_function().function().unoptimized_code());
On 2013/11/19 19:18:29, srdjan wrote:
> Maybe cache unoptimized code in class ParsedFunction when creating it instead
of
> using function as the proxy?

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1346: const Code& code =
Code::ZoneHandle(frame->LookupDartCode());
On 2013/11/19 19:18:29, srdjan wrote:
> Optional, as it will fail in next line: ASSERT(!code.IsNull())

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1349: ASSERT(function.HasCode());
On 2013/11/19 19:18:29, srdjan wrote:
> Why does this need to be true? Is this because once optimizations kick in, we
> cannot remove code? Please add brief comment, that would also cover the
> ASSERT(!original_code.IsNull()).

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1444:
caller_code.GetStaticCallTargetFunctionAt(frame->pc()));
On 2013/11/19 19:18:29, srdjan wrote:
> GetStaticCallTargetCodeAt ->target_code -> function

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:552: // IsInlineable above checked HasCode.
Creating a ZoneHandle for the code
On 2013/11/19 19:18:29, srdjan wrote:
> s/ZoneHandle/Handle/ since you stored it into a scoped handle not a zone
handle.

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:146: bool visit_function_code() { return
visit_function_code_; }
On 2013/11/19 19:18:29, srdjan wrote:
> const

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:170: TryDetachingCode();
On 2013/11/19 19:24:48, Ivan Posva wrote:
> DetachCode()

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:238: // If the code wasn't marked even after skipping
the function's code
On 2013/11/19 19:24:48, Ivan Posva wrote:
> // If the code wasn't strongly visited by other references, then we...
> 
> You can also leave the "It will be swept." out as that is a given for
Mark-Sweep
> and strictly speeking depends on the GC mechanism.

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:265: bool visit_function_code_;
On 2013/11/19 19:18:29, srdjan wrote:
> const

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:475: bool try_collecting_code) {
On 2013/11/19 19:24:48, Ivan Posva wrote:
> The "try" is unnecessary here. Either you collect code or you don't.

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/gc_marker.cc#...
runtime/vm/gc_marker.cc:476: bool visit_function_code = !try_collecting_code;
On 2013/11/19 19:18:29, srdjan wrote:
> const bool

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:6884: const Function& function_;
On 2013/11/19 19:18:29, srdjan wrote:
> Do we need function, should we use code_ to return function instead?

Removed.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/object.h#newc...
runtime/vm/object.h:1993: friend class RawFunction;
On 2013/11/19 19:24:48, Ivan Posva wrote:
> What is this needed for?

RawFunction::VisitFunctionPointers needs to access the private constructor of
Function. Added a comment.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:431: bool try_collecting_code = FLAG_collect_code &&
ShouldTryCollectingCode();
On 2013/11/19 19:24:48, Ivan Posva wrote:
> collect_code and ShouldCollectCode() please.

Done.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc
File runtime/vm/raw_object.cc (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc...
runtime/vm/raw_object.cc:382: if (fn.HasCode() &&  // Not already detached.
On 2013/11/19 19:24:48, Ivan Posva wrote:
> Wouldn't code just be null in this case?

Changed to !code.IsNull()

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.cc...
runtime/vm/raw_object.cc:385: (fn.usage_counter() > 0)) {
On 2013/11/19 19:24:48, Ivan Posva wrote:
> Is this still necessary? If it is please comment why exactly it is needed.

Changed to >= 0.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:614: friend class MarkingVisitor;
On 2013/11/19 19:24:48, Ivan Posva wrote:
> Why is this needed?

So that MarkingVisitor::DetachCode can null out the code fields.

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/visitor.h
File runtime/vm/visitor.h (right):

https://codereview.chromium.org/70183010/diff/630001/runtime/vm/visitor.h#new...
runtime/vm/visitor.h:28: virtual bool visit_function_code() { return true; }
On 2013/11/19 19:18:29, srdjan wrote:
> const 

Done.

[Ivan Posva, 2013-11-22 23:53:31]

LGTM -ip

[zra, 2013-11-23 00:17:54]

Committed patchset #8 manually as r30600 (presubmit successful).