Commit 8a3a5c77 authored by Andrew Grieve's avatar Andrew Grieve Committed by Commit Bot

Android: Change desugar to not cache stateless lambdas

Saves 15kb of dex and 300 dex methods by removing the <clinit>()
from them.

While it is sometimes a good idea to cache instances of inner
classes, there are several reasons why having it be automatic
is not a good idea:
* Since caching happens only for stateless lambdas, it is
  too subtle. I don't think most devs are mindful of stateful
  vs. stateless lambdas when creating / modifying them. The
  syntax is the same for both.
* Many lambdas are used only once. Adding in a <clinit>() adds
  synchronization overhead & an extra function call.

Bug: 952833

Change-Id: I9ab1474c93b2732954b6df5159ac89049f1bfcf1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902476
Auto-Submit: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: default avatarSam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714685}
parent 0319eb27
...@@ -791,7 +791,7 @@ deps = { ...@@ -791,7 +791,7 @@ deps = {
'packages': [ 'packages': [
{ {
'package': 'chromium/third_party/bazel', 'package': 'chromium/third_party/bazel',
'version': 'tQPvsIj1Gtw5iXssKy7OREE-S02u7zItrw42l3DHUroC', 'version': 'VjMsf48QUWw8n7XtJP2AuSjIGmbQeYdWdwyxVvIRLmAC',
}, },
], ],
'condition': 'checkout_android', 'condition': 'checkout_android',
......
...@@ -14,6 +14,7 @@ Googlers: See: go/desugar ...@@ -14,6 +14,7 @@ Googlers: See: go/desugar
Local Modifications: Local Modifications:
* Added BUILD.gn, proguard.flags. * Added BUILD.gn, proguard.flags.
* Made all lambdas be "stateful" to avoid <clinit> bloat.
* Desugar_deploy.jar split into Desugar.jar and Desugar_runtime.jar. * Desugar_deploy.jar split into Desugar.jar and Desugar_runtime.jar.
* Desugar.jar has been run through r8.jar to remove unused .class files. * Desugar.jar has been run through r8.jar to remove unused .class files.
...@@ -21,7 +22,9 @@ Update instructions (requires @google.com account for uploading): ...@@ -21,7 +22,9 @@ Update instructions (requires @google.com account for uploading):
* Check out Bazel from https://github.com/bazelbuild/bazel * Check out Bazel from https://github.com/bazelbuild/bazel
* Compile or install Bazel by following instructions on * Compile or install Bazel by following instructions on
https://docs.bazel.build/versions/master/install.html https://docs.bazel.build/versions/master/install.html
* Build Desugar_deploy.jar by running * Apply stateful-lambdas.patch:
git apply $CHROMIUM_SRC/third_party/bazel/desugar/stateful-lambdas.patch
* Build Desugar_deploy.jar:
bazel build //src/tools/android/java/com/google/devtools/build/android/desugar:Desugar_deploy.jar bazel build //src/tools/android/java/com/google/devtools/build/android/desugar:Desugar_deploy.jar
* Move Desugar_deploy.jar to location within Chromium: * Move Desugar_deploy.jar to location within Chromium:
rm $CHROMIUM_SRC/third_party/bazel/desugar/Desugar.jar rm $CHROMIUM_SRC/third_party/bazel/desugar/Desugar.jar
......
diff --git a/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaClassFixer.java b/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaClassFixer.java
index ff3e351996..f857e61d1e 100644
--- a/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaClassFixer.java
+++ b/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaClassFixer.java
@@ -93,7 +93,7 @@ class LambdaClassFixer extends ClassVisitor {
checkArgument(BitFlags.noneSet(access, Opcodes.ACC_INTERFACE), "Not a class: %s", name);
checkState(this.originalInternalName == null, "not intended for reuse but reused for %s", name);
originalInternalName = name;
- hasState = false;
+ hasState = true;
hasFactory = false;
desc = null;
this.signature = null;
@@ -136,7 +136,7 @@ class LambdaClassFixer extends ClassVisitor {
} else if ("<init>".equals(name)) {
this.desc = desc;
this.signature = signature;
- if (!lambdaInfo.needFactory() && !desc.startsWith("()")) {
+ if (!lambdaInfo.needFactory()) {
access &= ~Opcodes.ACC_PRIVATE; // make constructor accessible if we'll call it directly
}
}
@@ -156,10 +156,6 @@ class LambdaClassFixer extends ClassVisitor {
@Override
public void visitEnd() {
- checkState(
- !hasState || hasFactory,
- "Expected factory method for capturing lambda %s",
- getInternalName());
if (!hasState) {
checkState(
signature == null,
diff --git a/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaDesugaring.java b/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaDesugaring.java
index 8f90d25ff5..f5ed6d524a 100644
--- a/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaDesugaring.java
+++ b/src/tools/android/java/com/google/devtools/build/android/desugar/LambdaDesugaring.java
@@ -423,8 +423,7 @@ class LambdaDesugaring extends ClassVisitor {
String lambdaClassName = internalName + "$$Lambda$" + (lambdaCount++);
Type[] capturedTypes = Type.getArgumentTypes(desc);
boolean needFactory =
- capturedTypes.length != 0
- && !attemptAllocationBeforeArgumentLoads(lambdaClassName, capturedTypes);
+ !attemptAllocationBeforeArgumentLoads(lambdaClassName, capturedTypes);
lambdas.generateLambdaClass(
internalName,
LambdaInfo.create(
@@ -435,7 +434,7 @@ class LambdaDesugaring extends ClassVisitor {
bridgeInfo.bridgeMethod()),
bsmMethod,
args);
- if (desc.startsWith("()")) {
+ if (false) {
// For stateless lambda classes we'll generate a singleton instance that we can just load
checkState(capturedTypes.length == 0);
super.visitFieldInsn(
@@ -493,7 +492,6 @@ class LambdaDesugaring extends ClassVisitor {
* @return {@code true} if we were able to insert a new/dup, {@code false} otherwise
*/
private boolean attemptAllocationBeforeArgumentLoads(String internalName, Type[] paramTypes) {
- checkArgument(paramTypes.length > 0, "Expected at least one param for %s", internalName);
// Walk backwards past loads corresponding to constructor arguments to find the instruction
// after which we need to insert our NEW/DUP pair
AbstractInsnNode insn = instructions.getLast();
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment