Add deoptimization history to optimized functions.

Add deoptimization history to optimized functions.

Each function that is optimized or inlined into an optimized
functions keeps an array of deoptimization ids.

This history is used to avoid repeated deoptimization caused by
speculative hoisting of check instructions.

R=kmillikin@google.com

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

[Florian Schneider, 2013-05-24 08:13:20]

Redirecting to Kevin.

[Kevin Millikin (Google), 2013-05-24 11:39:21]

This is pretty sleazy but I kind of like it.

LGTM.

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

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:377: const Array& deopt_history =
Array::Handle(function.deopt_history());
You can use just one handle here.

Array& deopt_history = Array::Handle(function.deopt_history());
if (deopt_history.IsNull()) {
  deopt_history = Array::New(...);
  function.set_deopt_history(deopt_history);
}

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3507: if (obj.IsNull()) continue;
If you just delete this line, I think the result is exactly the same.

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3508: if (obj.IsSmi() &&
Smi::Cast(obj).Value() == target_deopt_id) {
Can it be non-smi (other than null)?  The Smi::Cast will assert it is.  If it's
always smi or null you can just use:

Smi& deopt_id = Smi::Handle();
deopt_id ^= deopt_history.At(i);
if (!deopt_id.IsNull() && (deopt_id.Value() == target)) {
  return false;
}

[Florian Schneider, 2013-05-24 11:53:02]

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

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:377: const Array& deopt_history =
Array::Handle(function.deopt_history());
On 2013/05/24 11:39:21, kmillikin wrote:
> You can use just one handle here.
> 
> Array& deopt_history = Array::Handle(function.deopt_history());
> if (deopt_history.IsNull()) {
>   deopt_history = Array::New(...);
>   function.set_deopt_history(deopt_history);
> }

Done.

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3507: if (obj.IsNull()) continue;
On 2013/05/24 11:39:21, kmillikin wrote:
> If you just delete this line, I think the result is exactly the same.

Done.

https://codereview.chromium.org/15779006/diff/17001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3508: if (obj.IsSmi() &&
Smi::Cast(obj).Value() == target_deopt_id) {
On 2013/05/24 11:39:21, kmillikin wrote:
> Can it be non-smi (other than null)?  The Smi::Cast will assert it is.  If
it's
> always smi or null you can just use:
> 
> Smi& deopt_id = Smi::Handle();
> deopt_id ^= deopt_history.At(i);
> if (!deopt_id.IsNull() && (deopt_id.Value() == target)) {
>   return false;
> }

Done.

[Florian Schneider, 2013-05-24 12:03:51]

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

[srdjan, 2013-05-28 10:48:48]

DBC

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/flow_graph_inl...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:620: function.set_deopt_history(deopt_history);
This code is used repeatedly. How about factoring it out into
Function::NeedsDeoptHistory() ?

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

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:618: int16_t deoptimization_counter_;
Please document what negative counter means.

[Florian Schneider, 2013-05-29 08:50:20]

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/flow_graph_inl...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:620: function.set_deopt_history(deopt_history);
On 2013/05/28 10:48:49, srdjan wrote:
> This code is used repeatedly. How about factoring it out into
> Function::NeedsDeoptHistory() ?

Good idea.

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

https://codereview.chromium.org/15779006/diff/28002/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:618: int16_t deoptimization_counter_;
On 2013/05/28 10:48:49, srdjan wrote:
> Please document what negative counter means.

It should never be negative. I just made this consistent with the getter/setter
that expect int16_t. On the other hand we use intptr_t in many places even
though the value is always positive.