Allow optimized code when debugger is active

Allow optimized code when debugger is active

The debugger allows the VM to optimize code unless there is a breakpoint inside the
function. Whenever a new breakpoint is set (explicitly or implicitly by stepping)
all optimized code is discarded.
Committed: https://code.google.com/p/dart/source/detail?r=16815

[srdjan, 2013-01-08 18:03:57]

LGTM

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

https://codereview.chromium.org/11780005/diff/1/runtime/vm/code_generator.cc#...
runtime/vm/code_generator.cc:1347: if
(isolate->debugger()->HasBreakpoint(function)) {
function.HasBreakpoint() instead.

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

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:765: // Deoptimize all functions on in stack activation
frames.
Fix comment.

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:766: DeoptimizeAll();
Add a TODO to deoptimize only functions that have breakpoints (incl. inlined
functions with breakpoints).

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:768: const ClassTable& class_table =
*isolate_->class_table();
Add a TODO to factor it out with  RemoveOptimizedCode

https://codereview.chromium.org/11780005/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_arm.h (right):

https://codereview.chromium.org/11780005/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_arm.h:37: bool CanOptimizeFunction();
const

[hausner, 2013-01-08 18:37:57]

Thank you for the review!

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

https://codereview.chromium.org/11780005/diff/1/runtime/vm/code_generator.cc#...
runtime/vm/code_generator.cc:1347: if
(isolate->debugger()->HasBreakpoint(function)) {
On 2013/01/08 18:03:57, srdjan wrote:
> function.HasBreakpoint() instead.

May look nicer but it costs a superfluous call to Isolate::Current(). I'd rather
keep it this way.

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

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:765: // Deoptimize all functions on in stack activation
frames.
On 2013/01/08 18:03:57, srdjan wrote:
> Fix comment.

Done.

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:766: DeoptimizeAll();
On 2013/01/08 18:03:57, srdjan wrote:
> Add a TODO to deoptimize only functions that have breakpoints (incl. inlined
> functions with breakpoints).

Done.

https://codereview.chromium.org/11780005/diff/1/runtime/vm/debugger.cc#newcod...
runtime/vm/debugger.cc:768: const ClassTable& class_table =
*isolate_->class_table();
On 2013/01/08 18:03:57, srdjan wrote:
> Add a TODO to factor it out with  RemoveOptimizedCode

Done.

https://codereview.chromium.org/11780005/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_arm.h (right):

https://codereview.chromium.org/11780005/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_arm.h:37: bool CanOptimizeFunction();
On 2013/01/08 18:03:57, srdjan wrote:
> const

Done.

[Vyacheslav Egorov (Google), 2013-01-08 20:11:05]

DBC: do we have a good test coverage for this? I don't see any tests added.

Here are some cases that are of the biggest interest:

stepping out into optimized frame (potentially with multiple optimized frames
above it);
stepping out into optimized frame which corresponds to multiple non-optimized
frames (inlining);
multiple optimized versions of the same function on the stack;
additionally it might be good to get some GCs in between.

[hausner, 2013-01-08 20:37:16]

I did a manual check to verify the expected outcome.

The cost of creating an automated test for this and maintaining it, making sure
all the assumptions that go into it still hold in the future, seems prohibitive.
Measuring execution time as the pass/fail criteria also makes tests flaky.

What we could do is run performance tests (at least one) with a stress flag that
simulates the existence of a breakpoint. But I still don't think it's worth it.