VM: Enable collection of unoptimized code for optimized functions.

VM: Enable collection of unoptimized code for optimized functions.

Until now, code GC was limited to function that have not been optimized.
With lazy generation of unoptimized code on deoptimization we can now
collect unoptimized code for optimized functions as well.

R=vegorov@google.com

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

[Florian Schneider, 2015-04-08 13:27:36]

fschneider@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2015-04-09 12:23:08]

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:981: if (optimized) {
Why do we need to restore unoptimized code for optimized compilation?

If it's only for edge counters then they'll be zeroed?

We need to sanitize that.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:688:
Compiler::EnsureUnoptimizedCode(Thread::Current(), function);
Same question here.

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

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:152: return &skipped_current_code_functions_;
I think there is no reason to have two separate lists. Just use the same list
and check if any of the two code objects need to be dropped.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5110: Compiler::EnsureUnoptimizedCode(Thread::Current(),
*this);
Comment above no longer applies because previously compiled could have been
discarded so we are going to compiler now.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5560: if ((unoptimized_code() != Object::null()) &&
if (x) { return false } else { return true } 

can be written as

return !x.

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

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:416: bool RawFunction::SkipCode(RawFunction* raw_fun,
bool visit_current_code) {
The name is *very* confusing. Rename it do something like CheckUsageCounter or
DecayUsageCounter.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:439: if (FLAG_always_drop_code || (fn.usage_counter()
== 0)) {
return (FLAG_always_drop_code || ...);

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:450: if (visitor->visit_function_code() ||
!SkipCode(raw_obj, true)) {
I think bool parameter is a bit confusing. Maybe pass RawCode down? We are in
the middle of GC anyways.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:457: if (visitor->visit_function_code() ||
!SkipCode(raw_obj, false)) {
we decay fn.usage_counter() twice. I think it should be decayed only once per
VisitFunctionPointers invocation.

[Florian Schneider, 2015-04-09 14:48:38]

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:981: if (optimized) {
On 2015/04/09 12:23:07, Vyacheslav Egorov (Google) wrote:
> Why do we need to restore unoptimized code for optimized compilation?
> 
> If it's only for edge counters then they'll be zeroed?
> 
> We need to sanitize that.

Agree. Do you want me to do that in this CL?

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5110: Compiler::EnsureUnoptimizedCode(Thread::Current(),
*this);
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> Comment above no longer applies because previously compiled could have been
> discarded so we are going to compiler now.

Done.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5560: if ((unoptimized_code() != Object::null()) &&
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> if (x) { return false } else { return true } 
> 
> can be written as
> 
> return !x.

Done.

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

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:416: bool RawFunction::SkipCode(RawFunction* raw_fun,
bool visit_current_code) {
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> The name is *very* confusing. Rename it do something like CheckUsageCounter or
> DecayUsageCounter.

Done.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:439: if (FLAG_always_drop_code || (fn.usage_counter()
== 0)) {
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> return (FLAG_always_drop_code || ...);

Done.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:450: if (visitor->visit_function_code() ||
!SkipCode(raw_obj, true)) {
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> I think bool parameter is a bit confusing. Maybe pass RawCode down? We are in
> the middle of GC anyways.

Done.

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:457: if (visitor->visit_function_code() ||
!SkipCode(raw_obj, false)) {
On 2015/04/09 12:23:08, Vyacheslav Egorov (Google) wrote:
> we decay fn.usage_counter() twice. I think it should be decayed only once per
> VisitFunctionPointers invocation.
> 

Done.

[Florian Schneider, 2015-04-09 14:58:43]

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

https://codereview.chromium.org/1067383002/diff/20001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:152: return &skipped_current_code_functions_;
On 2015/04/09 12:23:07, Vyacheslav Egorov (Google) wrote:
> I think there is no reason to have two separate lists. Just use the same list
> and check if any of the two code objects need to be dropped.

Done.

[Vyacheslav Egorov (Google), 2015-04-09 15:03:47]

lgtm

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

https://codereview.chromium.org/1067383002/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:298: GrowableArray<RawFunction*> skipped_functions_;
I think the old name was better. We skipped visiting some internal pointers not
functions themselves.

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

https://codereview.chromium.org/1067383002/diff/60001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:424: bool RawFunction::ShouldSkipCode(RawCode*
raw_code) {
I would call it ShouldVisitCode instead and invert results.

[Florian Schneider, 2015-04-09 15:07:50]

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

https://codereview.chromium.org/1067383002/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:298: GrowableArray<RawFunction*> skipped_functions_;
On 2015/04/09 15:03:46, Vyacheslav Egorov (Google) wrote:
> I think the old name was better. We skipped visiting some internal pointers
not
> functions themselves.

Done.

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

https://codereview.chromium.org/1067383002/diff/60001/runtime/vm/raw_object.c...
runtime/vm/raw_object.cc:424: bool RawFunction::ShouldSkipCode(RawCode*
raw_code) {
On 2015/04/09 15:03:46, Vyacheslav Egorov (Google) wrote:
> I would call it ShouldVisitCode instead and invert results.

Done.

[Florian Schneider, 2015-04-10 12:02:41]

Committed patchset #6 (id:100001) manually as r45062 (presubmit successful).