Don't inline methods whose basic blocks end with a throw
If the basic block will end up throwing, it is commonly not in the
critical path. If we throw, we incur in a performance cost anyway
so we can skip inlining those methods. Additionally, methods
before a throw are sometimes construct information which is
something we are not interested in inlining.
Note that this CL doesn't stop inlining for methods that eventually
always end with a throw. See the 2243- test for an example
(testEndsWithThrowButNotDirectly). We could perform a more detailed
analysis but that analysis will increase compile time so it is left
as a further optimization if needed.
Locally in a Pixel 5 with AOSP, code size improved:
* AGSA: 15.3 MB (~4.6%)
* System Server: 1.9 MB (~3.74%)
* SysemUIGoogle: 0.88MB (~3.05%)
Bug: 252884414
Bug: 256052088
Bug: 255984757
Bug: 227283224
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Change-Id: Id0b7894c0d63591e3b354520a47252bf8b91f44f
diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc
index 0dc4256..fe398b0 100644
--- a/compiler/optimizing/inliner.cc
+++ b/compiler/optimizing/inliner.cc
@@ -1469,9 +1469,9 @@
return true;
}
-// Returns whether our resource limits allow inlining this method.
-bool HInliner::IsInliningBudgetAvailable(ArtMethod* method,
- const CodeItemDataAccessor& accessor) const {
+bool HInliner::IsInliningEncouraged(const HInvoke* invoke_instruction,
+ ArtMethod* method,
+ const CodeItemDataAccessor& accessor) const {
if (CountRecursiveCallsOf(method) > kMaximumNumberOfRecursiveCalls) {
LOG_FAIL(stats_, MethodCompilationStat::kNotInlinedRecursiveBudget)
<< "Method "
@@ -1491,6 +1491,13 @@
return false;
}
+ if (invoke_instruction->GetBlock()->GetLastInstruction()->IsThrow()) {
+ LOG_FAIL(stats_, MethodCompilationStat::kNotInlinedEndsWithThrow)
+ << "Method " << method->PrettyMethod()
+ << " is not inlined because its block ends with a throw";
+ return false;
+ }
+
return true;
}
@@ -1557,7 +1564,7 @@
return false;
}
- if (!IsInliningBudgetAvailable(method, accessor)) {
+ if (!IsInliningEncouraged(invoke_instruction, method, accessor)) {
return false;
}
diff --git a/compiler/optimizing/inliner.h b/compiler/optimizing/inliner.h
index 581f524..0c36a8a 100644
--- a/compiler/optimizing/inliner.h
+++ b/compiler/optimizing/inliner.h
@@ -133,12 +133,14 @@
const CodeItemDataAccessor& accessor) const
REQUIRES_SHARED(Locks::mutator_lock_);
- // Returns whether the inlining budget allows inlining method.
+ // Returns whether inlining is encouraged.
//
// For example, this checks whether the function has grown too large and
// inlining should be prevented.
- bool IsInliningBudgetAvailable(art::ArtMethod* method, const CodeItemDataAccessor& accessor) const
- REQUIRES_SHARED(Locks::mutator_lock_);
+ bool IsInliningEncouraged(const HInvoke* invoke_instruction,
+ art::ArtMethod* method,
+ const CodeItemDataAccessor& accessor) const
+ REQUIRES_SHARED(Locks::mutator_lock_);
// Inspects the body of a method (callee_graph) and returns whether it can be
// inlined.
diff --git a/compiler/optimizing/optimizing_compiler_stats.h b/compiler/optimizing/optimizing_compiler_stats.h
index 24a28e3..96dfdac 100644
--- a/compiler/optimizing/optimizing_compiler_stats.h
+++ b/compiler/optimizing/optimizing_compiler_stats.h
@@ -102,6 +102,7 @@
kNotInlinedNotCompilable,
kNotInlinedNotVerified,
kNotInlinedCodeItem,
+ kNotInlinedEndsWithThrow,
kNotInlinedWont,
kNotInlinedRecursiveBudget,
kNotInlinedPolymorphicRecursiveBudget,
diff --git a/test/2243-checker-not-inline-into-throw/expected-stderr.txt b/test/2243-checker-not-inline-into-throw/expected-stderr.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/2243-checker-not-inline-into-throw/expected-stderr.txt
diff --git a/test/2243-checker-not-inline-into-throw/expected-stdout.txt b/test/2243-checker-not-inline-into-throw/expected-stdout.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/2243-checker-not-inline-into-throw/expected-stdout.txt
diff --git a/test/2243-checker-not-inline-into-throw/info.txt b/test/2243-checker-not-inline-into-throw/info.txt
new file mode 100644
index 0000000..a2ded81
--- /dev/null
+++ b/test/2243-checker-not-inline-into-throw/info.txt
@@ -0,0 +1 @@
+Tests that we don't inline methods if their basic blocks end with a throw.
diff --git a/test/2243-checker-not-inline-into-throw/src/Main.java b/test/2243-checker-not-inline-into-throw/src/Main.java
new file mode 100644
index 0000000..6f1280c
--- /dev/null
+++ b/test/2243-checker-not-inline-into-throw/src/Main.java
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ try {
+ $noinline$testEndsWithThrow();
+ throw new Exception("Unreachable");
+ } catch (Error expected) {
+ }
+
+ try {
+ $noinline$testEndsWithThrowButNotDirectly();
+ throw new Exception("Unreachable");
+ } catch (Error expected) {
+ }
+ }
+
+ // Empty methods are easy to inline anywhere.
+ private static void easyToInline() {}
+ private static void $inline$easyToInline() {}
+
+ /// CHECK-START: int Main.$noinline$testEndsWithThrow() inliner (before)
+ /// CHECK: InvokeStaticOrDirect method_name:Main.easyToInline
+
+ /// CHECK-START: int Main.$noinline$testEndsWithThrow() inliner (after)
+ /// CHECK: InvokeStaticOrDirect method_name:Main.easyToInline
+ static int $noinline$testEndsWithThrow() {
+ easyToInline();
+ throw new Error("");
+ }
+
+ // Currently we only stop inlining if the method's basic block ends with a throw. We do not stop
+ // inlining for methods that eventually always end with a throw.
+ static int $noinline$testEndsWithThrowButNotDirectly() {
+ $inline$easyToInline();
+ if (justABoolean) {
+ $inline$easyToInline();
+ } else {
+ $inline$easyToInline();
+ }
+ throw new Error("");
+ }
+
+ static boolean justABoolean;
+}
diff --git a/test/536-checker-needs-access-check/src2/other/InaccessibleClass.java b/test/536-checker-needs-access-check/src2/other/InaccessibleClass.java
index 646cc7e..6fd99ef 100644
--- a/test/536-checker-needs-access-check/src2/other/InaccessibleClass.java
+++ b/test/536-checker-needs-access-check/src2/other/InaccessibleClass.java
@@ -31,7 +31,7 @@
Class<?> klass = null;
try {
klass = GetInaccessibleClass.$inline$get();
- throw new Error("Unreachable");
+ System.out.println("Unreachable");
} catch (IllegalAccessError expected) {}
return klass;
}
diff --git a/test/639-checker-code-sinking/src/Main.java b/test/639-checker-code-sinking/src/Main.java
index 27df41f..f561718 100644
--- a/test/639-checker-code-sinking/src/Main.java
+++ b/test/639-checker-code-sinking/src/Main.java
@@ -710,6 +710,18 @@
// `NewArray` (and maybe from `LoadClass`). However, code sinking was pruning
// the environment of the `NewArray`, leading to a crash when compiling the
// code below on the device (we do not inline `core-oj` on host). b/252799691
+
+ // We currently have a heuristic that disallows inlining methods if their basic blocks end with a
+ // throw. We could add code so that `requireNonNull`'s block doesn't end with a throw but that
+ // would mean that the string builder optimization wouldn't fire as it requires all uses to be in
+ // the same block. If `requireNonNull` is inlined at some point, we need to re-mark it as $inline$
+ // so that the test is operational again.
+
+ /// CHECK-START: void Main.$noinline$twoThrowingPathsAndStringBuilderAppend(java.lang.Object) inliner (before)
+ /// CHECK: InvokeStaticOrDirect method_name:Main.requireNonNull
+
+ /// CHECK-START: void Main.$noinline$twoThrowingPathsAndStringBuilderAppend(java.lang.Object) inliner (after)
+ /// CHECK: InvokeStaticOrDirect method_name:Main.requireNonNull
private static void $noinline$twoThrowingPathsAndStringBuilderAppend(Object o) {
String s1 = "s1";
String s2 = "s2";
@@ -722,14 +734,14 @@
// `StringBuilderAppend` pattern recognition.
// (But that does not happen when the `StringBuilder` constructor is
// not inlined, see above.)
- $inline$requireNonNull(o);
+ requireNonNull(o);
String s1s2 = sb.append(s1).append(s2).toString();
sb = null;
throw new Error(s1s2);
}
- private static void $inline$requireNonNull(Object o) {
+ private static void requireNonNull(Object o) {
if (o == null) {
throw new Error("Object is null");
}
diff --git a/test/979-const-method-handle/src/Main.java b/test/979-const-method-handle/src/Main.java
index 04d782b..72d529b 100644
--- a/test/979-const-method-handle/src/Main.java
+++ b/test/979-const-method-handle/src/Main.java
@@ -267,14 +267,14 @@
// inline the loading.
try {
$inline$getPrivateField();
- throw new Error("Expected IllegalAccessError");
+ System.out.println("Expected IllegalAccessError");
} catch (IllegalAccessError e) {
// expected
}
try {
$inline$missingType();
- throw new Error("Expected NoClassDefFoundError");
+ System.out.println("Expected NoClassDefFoundError");
} catch (NoClassDefFoundError e) {
// expected
}