Support for inlining small methods (independent of they're called inside a loop or not)

Support for inlining small methods (independent of they're called inside a loop or not)

BUG=http://dartbug.com/11478
R=ngeoffray@google.com

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

[kustermann, 2013-07-15 17:40:27]

Request for initial comments.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1272: if
(inliningStack.any((entry) => entry.function == function))
I'll add a test to make sure this guard against recursive inlining works.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1279: //if
(inliningStack.length >= MAX_INLINING_DEPTH) {
Don't know if we should limit the inlining depth here (we may want to inline
small functions always).

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.status
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.statu...
tests/co19/co19-dart2js.status:75: Language/12_Statements/10_Try_A06_t01: Fail #
co19 issue: dart2js inlining results in stack traces not as accurate
This test tries to match an exact stacktrace. But if we're inlining, we may not
have as many frames.

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
tests/language/language_dart2js.status:155: #method_override5_test: Fail # Issue
11496
Don't know yet, why this test starts passing.

[ngeoffray, 2013-07-16 08:04:45]

Overall looking good! A few comments.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:139: bool
canInline(FunctionElement element, {bool insideLoop: false}) {
I'd drop the default value: it reads easier if all callers do provide the
argument. If they don't they'll get a checked mode error.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:142: return
canBeInlined[element];
nit: use return insideLoop ? ... : ...;

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:145: void
markAsInlinable(FunctionElement element, {bool insideLoop: false}) {
ditto for insideLoop argument.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:156: void
markAsNonInlinable(FunctionElement element, {bool insideLoop: false}) {
ditto.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:932: static const
MAX_INLINING_DEPTH = 5;
Consider moving inlining considerations into its own class.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1232: var insideLoop
= loopNesting > 0 || graph.calledInLoop;
var -> bool

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1271: // Don't inline
recursivly
Shouldn't that be part of the heuristics?

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1272: if
(inliningStack.any((entry) => entry.function == function))
Nit: add braces with a newline on an if.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1279: //if
(inliningStack.length >= MAX_INLINING_DEPTH) {
On 2013/07/15 17:40:27, kustermann wrote:
> Don't know if we should limit the inlining depth here (we may want to inline
> small functions always).

Should we have a canBeInlined cache for methods that are inlined after
MAX_INLINING_DEPTH? This cache would be filled with small methods, smaller than
the current INLINING_NODES constants.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1284: return
cachedCanBeInlined;
One line, or braces.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1286: var
numParameters = function.functionSignature.parameterCount;
var -> int

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1287: var
maxInliningNodes;
var -> int.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1295: var
canBeInlined = InlineWeeder.canBeInlined(
var -> bool

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:5150: int
maxInliningNodes;
final

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.status
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.statu...
tests/co19/co19-dart2js.status:75: Language/12_Statements/10_Try_A06_t01: Fail #
co19 issue: dart2js inlining results in stack traces not as accurate
On 2013/07/15 17:40:27, kustermann wrote:
> This test tries to match an exact stacktrace. But if we're inlining, we may
not
> have as many frames.

File a bug?

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
tests/language/language_dart2js.status:155: #method_override5_test: Fail # Issue
11496
On 2013/07/15 17:40:27, kustermann wrote:
> Don't know yet, why this test starts passing.

From roughly looking at it, I think it's because we have a bug at runtime with
overrides and named parameters. We implement the correct substitution when
inlining, so we inline the right method with your CL. But if we don't inline it,
at runtime we hit the wrong method. The bug is on my radar.

[kustermann, 2013-07-16 13:27:50]

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:139: bool
canInline(FunctionElement element, {bool insideLoop: false}) {
On 2013/07/16 08:04:45, ngeoffray wrote:
> I'd drop the default value: it reads easier if all callers do provide the
> argument. If they don't they'll get a checked mode error.

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:142: return
canBeInlined[element];
On 2013/07/16 08:04:45, ngeoffray wrote:
> nit: use return insideLoop ? ... : ...;

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:145: void
markAsInlinable(FunctionElement element, {bool insideLoop: false}) {
On 2013/07/16 08:04:45, ngeoffray wrote:
> ditto for insideLoop argument.

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:156: void
markAsNonInlinable(FunctionElement element, {bool insideLoop: false}) {
On 2013/07/16 08:04:45, ngeoffray wrote:
> ditto.

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:932: static const
MAX_INLINING_DEPTH = 5;
On 2013/07/16 08:04:45, ngeoffray wrote:
> Consider moving inlining considerations into its own class.

Moved it now to InlineWeeder.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1232: var insideLoop
= loopNesting > 0 || graph.calledInLoop;
On 2013/07/16 08:04:45, ngeoffray wrote:
> var -> bool

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1271: // Don't inline
recursivly
On 2013/07/16 08:04:45, ngeoffray wrote:
> Shouldn't that be part of the heuristics?

Why should this be part of a heuristic? We require that the inliner doesn't run
into an infinite loop (=> hard constraint).

We could of course say that we allow recursive inlining up to a certain depth or
so. But I'm not sure if this would be worth it.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1272: if
(inliningStack.any((entry) => entry.function == function))
On 2013/07/16 08:04:45, ngeoffray wrote:
> Nit: add braces with a newline on an if.

I saw this style
  if (cond)
    return x;
quite a lot in the code, so I thought that's how the dart2js gods like it.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1279: //if
(inliningStack.length >= MAX_INLINING_DEPTH) {
I think we should not make our inlining decisions based on how deep the inlining
stack is. There are other things we should consider for the heuristics, but not
the inlining depth
 - guessing the number of call sites of a function (could be collected by the
type inference algorithm?)
 - ...

This code was probably here only to guard against running into a recursion.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1284: return
cachedCanBeInlined;
On 2013/07/16 08:04:45, ngeoffray wrote:
> One line, or braces.

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1286: var
numParameters = function.functionSignature.parameterCount;
On 2013/07/16 08:04:45, ngeoffray wrote:
> var -> int

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1287: var
maxInliningNodes;
On 2013/07/16 08:04:45, ngeoffray wrote:
> var -> int.

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1295: var
canBeInlined = InlineWeeder.canBeInlined(
On 2013/07/16 08:04:45, ngeoffray wrote:
> var -> bool

Done.

https://codereview.chromium.org/19250002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:5150: int
maxInliningNodes;
On 2013/07/16 08:04:45, ngeoffray wrote:
> final

Done.

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.status
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/co19/co19-dart2js.statu...
tests/co19/co19-dart2js.status:75: Language/12_Statements/10_Try_A06_t01: Fail #
co19 issue: dart2js inlining results in stack traces not as accurate
What I really would like is something like:

@DisableInlining()
void fun() {
}

So we can still have tests that rely on getting accurate stack traces. Do we
have something like that?

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/19250002/diff/1/tests/language/language_dart2...
tests/language/language_dart2js.status:155: #method_override5_test: Fail # Issue
11496
On 2013/07/16 08:04:45, ngeoffray wrote:
> On 2013/07/15 17:40:27, kustermann wrote:
> > Don't know yet, why this test starts passing.
> 
> From roughly looking at it, I think it's because we have a bug at runtime with
> overrides and named parameters. We implement the correct substitution when
> inlining, so we inline the right method with your CL. But if we don't inline
it,
> at runtime we hit the wrong method. The bug is on my radar.

Same here. If I remove this line from the status files, we don't have a single
test which fails on the runtime problem.
A "@DisableInlining()" annotation would here also enable us to test the runtime
functionality.

[kustermann, 2013-07-16 16:14:52]

PTAL

[ngeoffray, 2013-07-16 17:17:37]

LGTM!

https://codereview.chromium.org/19250002/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19250002/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1264: // Don't inline
recursivly
As discussed, removing the inlining depth is a heuristic, and that is why I
would remove this hard limitation into a heuristic too.

[kustermann, 2013-07-17 09:46:42]

https://codereview.chromium.org/19250002/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19250002/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1264: // Don't inline
recursivly
On 2013/07/16 17:17:37, ngeoffray wrote:
> As discussed, removing the inlining depth is a heuristic, and that is why I
> would remove this hard limitation into a heuristic too.

Done.

[kustermann, 2013-07-17 13:03:20]

Committed patchset #5 manually as r25093 (presubmit successful).