Disconnects code objects from infrequently used unoptimized functions.

Disconnects code objects from infrequently used unoptimized functions.

Every 30 seconds (configurable by --code-collection-interval),
before a MarkSweep collection, this change halves a function's
usage count if it is unoptimized. If the function's usage count
reaches 0 as a result of this halving, it sets the function's
code pointers to null. Then, if the code object isn't marked
during the MarkSweep, it will be collected.

This change also checks for null code pointers in various
places, and recompiles/reconnects code if needed.

R=srdjan@google.com

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

[zra, 2013-10-18 00:26:26]

Most of the new action is in page.cc. The other changes are for recompiling,
etc., when needed.

[srdjan, 2013-10-18 17:16:07]

More comments offline

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1116: __ j(NOT_EQUAL, &is_connected,
Assembler::kNearJump);
Add comment what you are doing: add the code object back to function?

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1120:
Assembler::kEntryPointToPcMarkerOffset;
Indent 4 chars the two lines above

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1454: __ popl(EAX);  // Restore function.
The argument can be modified in the callee, therefore you need to do extra
saving/restoring of function.

[hausner, 2013-10-18 17:46:17]

DBC

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

https://codereview.chromium.org/27802002/diff/1/runtime/vm/pages.cc#newcode399
runtime/vm/pages.cc:399: void CodeDropperVisitor::VisitObject(RawObject*
raw_obj) {
You should also check whether there are breakpoints set in the function. If
there are, you can't drop the code objects, otherwise the debugger will break.
Look at Debugger::HasBreakpoint(function).

[srdjan, 2013-10-18 18:04:32]

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

https://codereview.chromium.org/27802002/diff/1/runtime/vm/pages.cc#newcode406
runtime/vm/pages.cc:406: (fn.usage_counter() > 0)) {
Use !fn.HasOptimizedCode()

[zra, 2013-10-22 16:13:23]

As discussed, when code is detached, I now patch the code to be non-entrant.
Then, when the function is called, I reattach the code, and patch it to be
entrant again.

PTAL

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1116: __ j(NOT_EQUAL, &is_connected,
Assembler::kNearJump);
On 2013/10/18 17:16:07, srdjan wrote:
> Add comment what you are doing: add the code object back to function?

Code obsolete.

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1120:
Assembler::kEntryPointToPcMarkerOffset;
On 2013/10/18 17:16:07, srdjan wrote:
> Indent 4 chars the two lines above

Code obsolete

https://codereview.chromium.org/27802002/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:1454: __ popl(EAX);  // Restore function.
On 2013/10/18 17:16:07, srdjan wrote:
> The argument can be modified in the callee, therefore you need to do extra
> saving/restoring of function.

At other call-sites of CompileFunction (e.g. around stub_code_ia32.cc:720) there
are comments indicating the argument is read-only. If this is actually not true,
I'll preserve the function, and edit the comments at all call-sites.

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

https://codereview.chromium.org/27802002/diff/1/runtime/vm/pages.cc#newcode399
runtime/vm/pages.cc:399: void CodeDropperVisitor::VisitObject(RawObject*
raw_obj) {
On 2013/10/18 17:46:17, hausner wrote:
> You should also check whether there are breakpoints set in the function. If
> there are, you can't drop the code objects, otherwise the debugger will break.
> Look at Debugger::HasBreakpoint(function).

Added !fn.HasBreakpoint() to the conditions for detaching the code.

https://codereview.chromium.org/27802002/diff/1/runtime/vm/pages.cc#newcode406
runtime/vm/pages.cc:406: (fn.usage_counter() > 0)) {
On 2013/10/18 18:04:32, srdjan wrote:
> Use !fn.HasOptimizedCode()

Done.

[srdjan, 2013-10-23 15:58:13]

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1425: if (target_function.CurrentCode() ==
Code::null()) {
if (target_function.HasCode()) {
 .... old code: patch to new target
} else {
  ... reattach code
}

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1427: target_code ^=
caller_code.GetStaticCallTargetCodeAt(frame->pc());
ASSERT(!target_code.is_optimized())

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1428: ASSERT(target_code.raw() != Code::null());
ASSERT(!target_code.IsNull())

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:1378: __ EnterStubFrame();
I am not sure that we can do EnterStubFrame in compiled code. Typically we call
to a stub which does EnterStubFrame. This means that there must be a PC of
compiled code on the stack before doing EnterStubFrame.

Also: megamorphic calls are emitted only in optimized code, i.e., you will never
hit this since code is not cleared if optimized code exists.

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4084: void Function::set_unoptimized_code(const Code&
value) const {
Please add assert: !code.is_optimized().

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:407: (fn.CurrentCode() != Code::null()) &&  // Not already
detached.
fn.HasCode()

[zra, 2013-10-23 17:39:36]

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1425: if (target_function.CurrentCode() ==
Code::null()) {
On 2013/10/23 15:58:13, srdjan wrote:
> if (target_function.HasCode()) {
>  .... old code: patch to new target
> } else {
>   ... reattach code
> }

Done.

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1427: target_code ^=
caller_code.GetStaticCallTargetCodeAt(frame->pc());
On 2013/10/23 15:58:13, srdjan wrote:
> ASSERT(!target_code.is_optimized())

Done.

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:1428: ASSERT(target_code.raw() != Code::null());
On 2013/10/23 15:58:13, srdjan wrote:
> ASSERT(!target_code.IsNull())

Done.

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:1378: __ EnterStubFrame();
On 2013/10/23 15:58:13, srdjan wrote:
> I am not sure that we can do EnterStubFrame in compiled code. Typically we
call
> to a stub which does EnterStubFrame. This means that there must be a PC of
> compiled code on the stack before doing EnterStubFrame.
> 
> Also: megamorphic calls are emitted only in optimized code, i.e., you will
never
> hit this since code is not cleared if optimized code exists.

Can the megamorphic call be a call to unoptimized code? Because I am getting a
null code pointer here.

Either way, I've updated to use GenerateCallRuntime. It seems no new frame is
needed here, following the example of other runtime calls from generated code.

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4084: void Function::set_unoptimized_code(const Code&
value) const {
On 2013/10/23 15:58:13, srdjan wrote:
> Please add assert: !code.is_optimized().

Done.

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

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:407: (fn.CurrentCode() != Code::null()) &&  // Not already
detached.
On 2013/10/23 15:58:13, srdjan wrote:
> fn.HasCode()

Done.

[zra, 2013-10-23 23:27:51]

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/27802002/diff/195001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:1378: __ EnterStubFrame();
On 2013/10/23 17:39:37, zra wrote:
> On 2013/10/23 15:58:13, srdjan wrote:
> > I am not sure that we can do EnterStubFrame in compiled code. Typically we
> call
> > to a stub which does EnterStubFrame. This means that there must be a PC of
> > compiled code on the stack before doing EnterStubFrame.
> > 
> > Also: megamorphic calls are emitted only in optimized code, i.e., you will
> never
> > hit this since code is not cleared if optimized code exists.
> 
> Can the megamorphic call be a call to unoptimized code? Because I am getting a
> null code pointer here.
> 
> Either way, I've updated to use GenerateCallRuntime. It seems no new frame is
> needed here, following the example of other runtime calls from generated code.

Moved call to a stub.

[srdjan, 2013-10-24 16:33:30]

LGTM with comments.

Please run test suite with a short collection interval and unoptimized code only
(--optimization-counter-threshold=-1)

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

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:28: DEFINE_FLAG(int, code_collection_interval, 500000,
code_collection_interval_in_us

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.h#newco...
runtime/vm/pages.h:162: int64_t last_code_collection_;
I like specifying the unit in name: last_code_collection_in_us_

[zra, 2013-10-24 20:53:09]

Running tests.

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

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:28: DEFINE_FLAG(int, code_collection_interval, 500000,
On 2013/10/24 16:33:30, srdjan wrote:
> code_collection_interval_in_us

Done.

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/27802002/diff/355001/runtime/vm/pages.h#newco...
runtime/vm/pages.h:162: int64_t last_code_collection_;
On 2013/10/24 16:33:30, srdjan wrote:
> I like specifying the unit in name: last_code_collection_in_us_

Done.

[zra, 2013-10-24 21:04:26]

On 2013/10/24 20:53:09, zra wrote:
> Running tests.

Running with optimization disabled and a code collection interval of 100ms,
several tests time-out in each configuration, even with a timeout of 480
seconds.

[zra, 2013-10-24 21:26:29]

Committed patchset #6 manually as r29209 (presubmit successful).