Fix issue 21159: prevent endless inlining of field dispatchers.

Fix issue 21159: prevent endless inlining of field dispatchers.

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

[srdjan, 2014-10-15 20:41:35]

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

[srdjan, 2014-10-15 20:41:36]

Do you have a better suggestion? Why did you not use inlining_depth_  for
InvokeFieldDispatcher?

[Florian Schneider, 2014-10-16 10:29:18]

On 2014/10/15 20:41:36, srdjan wrote:
> Do you have a better suggestion? Why did you not use inlining_depth_  for
> InvokeFieldDispatcher?

We could just disable recursive inlining. It usually does not pay off (except
for micro-benchmarks).

[Florian Schneider, 2014-10-16 10:34:33]

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

https://codereview.chromium.org/659793003/diff/40001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:767: if ((GrowthFactor() < 10.0) &&
We need to avoid infinite recursion, but not sure if this is the best way: The
idea is to always inline those dispatchers since they are small and should not
get into the way when generating optimized code.

[srdjan, 2014-10-16 17:12:26]

Disabled recursive inlining.

[srdjan, 2014-10-16 17:13:06]

Committed patchset #4 (id:60001) manually as r41156 (presubmit successful).

[zerny-google, 2014-10-20 11:28:20]

zerny@google.com changed reviewers:
+ zerny@google.com

[zerny-google, 2014-10-20 11:28:21]

So far as we need to cater to the micro benchmarks...

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

https://codereview.chromium.org/659793003/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:773: 0,
DBC: It seems this issue is caused by ignoring the inlining depth. We could
separate the general depth threshold from the recursive depth threshold (I
recall talking about this, but not implementing it, since, at the time, the
general depth achieved mostly the same thing.)

[Florian Schneider, 2014-10-20 14:01:57]

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

https://codereview.chromium.org/659793003/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:617: function.set_is_inlinable(false);
Maybe that should not mark the whole function as not inlinable in this case:

I found that for example the Integer.compareTo could still be inlined into its
caller, but the recursive call-site inside compareTo (which may not be reachable
after constant propagation) should not be inlined.

https://codereview.chromium.org/659793003/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:773: 0,
On 2014/10/20 11:28:21, zerny-google wrote:
> DBC: It seems this issue is caused by ignoring the inlining depth. We could
> separate the general depth threshold from the recursive depth threshold (I
> recall talking about this, but not implementing it, since, at the time, the
> general depth achieved mostly the same thing.)

I agree that a separate threshold for recursive inlining would be the best if we
do want to inline recursive call-sites. Dispatchers should always be inlined,
since they are so small.