Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558633007:
Improve inlining heuristics. (Closed)
|
Created:
4 years ago by floitsch Modified:
3 years, 9 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImprove inlining heuristics.
Patch Set 1 #
Total comments: 11
Messages
Total messages: 9 (1 generated)
sigmund@google.com changed reviewers: + sigmund@google.com, sra@google.com
+sra cool Florian. I really like the simplicity of this. I'll test this out on some large apps to see the effect there. Have you applied this patch in our perf bots to see the general effect there? If not, I'm happy to do it for you, just let me know.
https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6692: /* Enabling this, slightly increases the output size. I think we can play a bit with the max-sizes, though. If we count references to the arguments as less important, then we can try to decrease the overall inlining size.
I'm a little wary about changing the inlining heuristics. If there is a specific library function that should be inlined, use the @ForceInline() annotation, and then that will be reliable when we implement inlining for Kernel-ir.
On 2017/01/04 00:29:00, sra1 wrote: > I'm a little wary about changing the inlining heuristics. > If there is a specific library function that should be inlined, use the > @ForceInline() annotation, and then that will be reliable when we implement > inlining for Kernel-ir. Given that the inlining heuristics are not very well tuned I don't understand why you are against changing it. Clearly, we would need to test this change on more than dart2js, but there it was both inlining more, and at the same time reducing the code size.
I can't patch, so can you rebase? https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6696: // argument would cost us as much space as we inline. It will cost a temporary if it is used twice. Can we count uses? https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6719: int oldCount = nodeCount; I think this code would be easier to understand if this was called commonPrefixCount, and the then/else were explicitly computed. https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6722: int thenCount = nodeCount; int thenCount = nodeCount - commonPrefixCount ... int elseCount = nodeCount - commonPrefixCount nodeCount = commonPrefixCount + max(thenCount, elseCount); ... nodeCount = commonPrefixCount + thenCount + elseCount; https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6726: if (node.condition.asSend() != null && Add a comment that this is a heuristic - a parameter directly used in a conditional is likely to be a constant, so count only the biggest arm.
I get this on a certain large app: The compiler crashed: Unsupported operation: ResolvedAstKind.DEFAULT_CONSTRUCTOR:generative_constructor(BrowserDomAdapter#) does not provide a TreeElements #0 SynthesizedResolvedAst.elements (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/elements/elements.dart:1927:5) #1 SsaBuilder.elements (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:237:44) #2 SsaBuilder.tryInlineMethod.heuristicSayGoodToGo (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:547:11) #3 SsaBuilder.tryInlineMethod (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:585:55) #4 SsaBuilder.buildFactory (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:1352:11) #5 SsaBuilder.visitInlinedFunction (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:866:7) #6 SsaBuilder.doInline (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:6421:5) #7 SsaBuilder.tryInlineMethod.doInlining.<anonymous closure> (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:579:11) #8 SsaBuilder.inlinedFrom.<anonymous closure> (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:619:21) #9 CompilerDiagnosticReporter.withCurrentElement (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/compiler.dart:1318:15) #10 SsaBuilder.inlinedFrom (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:612:21) #11 SsaBuilder.tryInlineMethod.doInlining (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:575:7) #12 SsaBuilder.tryInlineMethod (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:586:17) #13 SsaBuilder.pushInvokeStatic (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:4120:9) #14 SsaBuilder.handleNewSend (file:///usr/local/google/home/sra/Dart2/sdk/pkg/compiler/lib/src/ssa/builder.dart:3461:7)
https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6631: if (resolvedAst.elements.containsTryStatement) return false; elements does not need to be passed in, since it it resolvedAst.elements
Thanks. Stephen took over this CL here: https://codereview.chromium.org/2724473004/ https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6631: if (resolvedAst.elements.containsTryStatement) return false; On 2017/02/28 22:48:28, sra1 wrote: > elements does not need to be passed in, since it it resolvedAst.elements even better. https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6696: // argument would cost us as much space as we inline. On 2017/02/28 19:36:15, sra1 wrote: > It will cost a temporary if it is used twice. > Can we count uses? That's true, unless the parameter is a constant or already in a temporary. I do agree that we shouldn't just ignore sends to arguments, *but* they should be counted less than other variables. Lots of chances of optimizations when one can inline declarations to their uses. https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6719: int oldCount = nodeCount; On 2017/02/28 19:36:15, sra1 wrote: > I think this code would be easier to understand if this was called > commonPrefixCount, and the then/else were explicitly computed. Acknowledged. https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6722: int thenCount = nodeCount; On 2017/02/28 19:36:15, sra1 wrote: > int thenCount = nodeCount - commonPrefixCount > ... > int elseCount = nodeCount - commonPrefixCount > > > nodeCount = commonPrefixCount + max(thenCount, elseCount); > ... > nodeCount = commonPrefixCount + thenCount + elseCount; Acknowledged. https://codereview.chromium.org/2558633007/diff/1/pkg/compiler/lib/src/ssa/bu... pkg/compiler/lib/src/ssa/builder.dart:6726: if (node.condition.asSend() != null && On 2017/02/28 19:36:15, sra1 wrote: > Add a comment that this is a heuristic - a parameter directly used in a > conditional is likely to be a constant, so count only the biggest arm. Acknowledged. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
