From 950d20dcbc7760aa9c107c9e0c3e3e79ddc0d9ad Mon Sep 17 00:00:00 2001 From: cnsun Date: Sat, 4 Feb 2017 19:08:14 -0500 Subject: Optimize the runtime library for try-with-resources, by reducing the granularity of locks. Now it uses a customized concurrent weak identity hash map. RELNOTES: n/a PiperOrigin-RevId: 155688279 GitOrigin-RevId: 3bf15e757a801ff813370aaa01ebc9143a8834d4 Change-Id: I0e52abcd7979e59f22be76f37379b06cc470f343 --- .../android/desugar/TryWithResourcesRewriter.java | 2 + .../desugar/runtime/ThrowableExtension.java | 158 ++++++++++++++++----- 2 files changed, 126 insertions(+), 34 deletions(-) diff --git a/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java b/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java index 38255a1..4093e12 100644 --- a/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java +++ b/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java @@ -52,6 +52,8 @@ public class TryWithResourcesRewriter extends ClassVisitor { ImmutableSet.of( THROWABLE_EXTENSION_INTERNAL_NAME, THROWABLE_EXTENSION_INTERNAL_NAME + "$AbstractDesugaringStrategy", + THROWABLE_EXTENSION_INTERNAL_NAME + "$ConcurrentWeakIdentityHashMap", + THROWABLE_EXTENSION_INTERNAL_NAME + "$ConcurrentWeakIdentityHashMap$WeakKey", THROWABLE_EXTENSION_INTERNAL_NAME + "$MimicDesugaringStrategy", THROWABLE_EXTENSION_INTERNAL_NAME + "$NullDesugaringStrategy", THROWABLE_EXTENSION_INTERNAL_NAME + "$ReuseDesugaringStrategy"); diff --git a/java/com/google/devtools/build/android/desugar/runtime/ThrowableExtension.java b/java/com/google/devtools/build/android/desugar/runtime/ThrowableExtension.java index 3581fe8..365884b 100644 --- a/java/com/google/devtools/build/android/desugar/runtime/ThrowableExtension.java +++ b/java/com/google/devtools/build/android/desugar/runtime/ThrowableExtension.java @@ -15,10 +15,13 @@ package com.google.devtools.build.android.desugar.runtime; import java.io.PrintStream; import java.io.PrintWriter; +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; import java.lang.reflect.Field; -import java.util.ArrayList; import java.util.List; -import java.util.WeakHashMap; +import java.util.Vector; +import java.util.concurrent.ConcurrentHashMap; /** * This is an extension class for java.lang.Throwable. It emulates the methods @@ -27,7 +30,7 @@ import java.util.WeakHashMap; * *

Note that the Desugar should avoid desugaring this class. */ -public class ThrowableExtension { +public final class ThrowableExtension { static final AbstractDesugaringStrategy STRATEGY; /** @@ -142,7 +145,7 @@ public class ThrowableExtension { } /** This strategy just delegates all the method calls to java.lang.Throwable. */ - static class ReuseDesugaringStrategy extends AbstractDesugaringStrategy { + static final class ReuseDesugaringStrategy extends AbstractDesugaringStrategy { @Override public void addSuppressed(Throwable receiver, Throwable suppressed) { @@ -171,17 +174,14 @@ public class ThrowableExtension { } /** This strategy mimics the behavior of suppressed exceptions with a map. */ - static class MimicDesugaringStrategy extends AbstractDesugaringStrategy { + static final class MimicDesugaringStrategy extends AbstractDesugaringStrategy { - public static final String SUPPRESSED_PREFIX = "Suppressed: "; - private final WeakHashMap> map = new WeakHashMap<>(); + static final String SUPPRESSED_PREFIX = "Suppressed: "; + private final ConcurrentWeakIdentityHashMap map = new ConcurrentWeakIdentityHashMap(); /** * Suppress an exception. If the exception to be suppressed is {@receiver} or {@null}, an * exception will be thrown. - * - * @param receiver - * @param suppressed */ @Override public void addSuppressed(Throwable receiver, Throwable suppressed) { @@ -191,23 +191,17 @@ public class ThrowableExtension { if (suppressed == null) { throw new NullPointerException("The suppressed exception cannot be null."); } - synchronized (this) { - List list = map.get(receiver); - if (list == null) { - list = new ArrayList<>(1); - map.put(receiver, list); - } - list.add(suppressed); - } + // The returned list is a synchrnozed list. + map.get(receiver, /*createOnAbsence=*/true).add(suppressed); } @Override - public synchronized Throwable[] getSuppressed(Throwable receiver) { - List list = map.get(receiver); + public Throwable[] getSuppressed(Throwable receiver) { + List list = map.get(receiver, /*createOnAbsence=*/false); if (list == null || list.isEmpty()) { return EMPTY_THROWABLE_ARRAY; } - return list.toArray(new Throwable[0]); + return list.toArray(EMPTY_THROWABLE_ARRAY); } /** @@ -218,35 +212,131 @@ public class ThrowableExtension { * {@code receiver} and its suppressed exceptions are printed in two different streams. */ @Override - public synchronized void printStackTrace(Throwable receiver) { + public void printStackTrace(Throwable receiver) { receiver.printStackTrace(); - for (Throwable suppressed : getSuppressed(receiver)) { - System.err.print(SUPPRESSED_PREFIX); - suppressed.printStackTrace(); + List suppressedList = map.get(receiver, /*createOnAbsence=*/false); + if (suppressedList == null) { + return; + } + synchronized (suppressedList) { + for (Throwable suppressed : suppressedList) { + System.err.print(SUPPRESSED_PREFIX); + suppressed.printStackTrace(); + } } } @Override - public synchronized void printStackTrace(Throwable receiver, PrintStream stream) { + public void printStackTrace(Throwable receiver, PrintStream stream) { receiver.printStackTrace(stream); - for (Throwable suppressed : getSuppressed(receiver)) { - stream.print(SUPPRESSED_PREFIX); - suppressed.printStackTrace(stream); + List suppressedList = map.get(receiver, /*createOnAbsence=*/false); + if (suppressedList == null) { + return; + } + synchronized (suppressedList) { + for (Throwable suppressed : suppressedList) { + stream.print(SUPPRESSED_PREFIX); + suppressed.printStackTrace(stream); + } } } @Override - public synchronized void printStackTrace(Throwable receiver, PrintWriter writer) { + public void printStackTrace(Throwable receiver, PrintWriter writer) { receiver.printStackTrace(writer); - for (Throwable suppressed : getSuppressed(receiver)) { - writer.print(SUPPRESSED_PREFIX); - suppressed.printStackTrace(writer); + List suppressedList = map.get(receiver, /*createOnAbsence=*/false); + if (suppressedList == null) { + return; + } + synchronized (suppressedList) { + for (Throwable suppressed : suppressedList) { + writer.print(SUPPRESSED_PREFIX); + suppressed.printStackTrace(writer); + } + } + } + } + + /** A hash map, that is concurrent, weak-key, and identity-hashing. */ + static final class ConcurrentWeakIdentityHashMap { + + private final ConcurrentHashMap> map = + new ConcurrentHashMap<>(16, 0.75f, 10); + private final ReferenceQueue referenceQueue = new ReferenceQueue<>(); + + /** + * @param throwable, the key to retrieve or create associated list. + * @param createOnAbsence {@code true} to create a new list if there is no value for the key. + * @return the associated value with the given {@code throwable}. If {@code createOnAbsence} is + * {@code true}, the returned value will be non-null. Otherwise, it can be {@code null} + */ + public List get(Throwable throwable, boolean createOnAbsence) { + deleteEmptyKeys(); + WeakKey keyForQuery = new WeakKey(throwable, null); + List list = map.get(keyForQuery); + if (!createOnAbsence) { + return list; + } + if (list != null) { + return list; + } + List newValue = new Vector<>(2); + list = map.putIfAbsent(new WeakKey(throwable, referenceQueue), newValue); + return list == null ? newValue : list; + } + + /** For testing-purpose */ + int size() { + return map.size(); + } + + void deleteEmptyKeys() { + // The ReferenceQueue.poll() is thread-safe. + for (Reference key = referenceQueue.poll(); key != null; key = referenceQueue.poll()) { + map.remove(key); + } + } + + private static final class WeakKey extends WeakReference { + + /** + * The hash code is used later to retrieve the entry, of which the key is the current weak + * key. If the referent is marked for garbage collection and is set to null, we are still able + * to locate the entry. + */ + private final int hash; + + public WeakKey(Throwable referent, ReferenceQueue q) { + super(referent, q); + if (referent == null) { + throw new NullPointerException("The referent cannot be null"); + } + hash = System.identityHashCode(referent); + } + + @Override + public int hashCode() { + return hash; + } + + @Override + public boolean equals(Object obj) { + if (obj == null || obj.getClass() != getClass()) { + return false; + } + if (this == obj) { + return true; + } + WeakKey other = (WeakKey) obj; + // Note that, after the referent is garbage collected, then the referent will be null. + // And the equality test still holds. + return this.hash == other.hash && this.get() == other.get(); } } } /** This strategy ignores all suppressed exceptions, which is how retrolambda does. */ - static class NullDesugaringStrategy extends AbstractDesugaringStrategy { + static final class NullDesugaringStrategy extends AbstractDesugaringStrategy { @Override public void addSuppressed(Throwable receiver, Throwable suppressed) { -- cgit v1.2.3 From 2488e5f7cdb4fc9f90271ffbde5f674b19751e47 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 12 May 2017 23:43:38 +0200 Subject: Fix Desugar duplicating path for dumped classes. When dumpDirectory (jdk.internal.lambda.dumpProxyClasses) is a relative path, Desugar was duplicating that path because it was resolving lambdaClass.getKey() (which contained already the the dumpDirectory path). Fixed by ensuring dumpDirectory is always an absolute path. RELNOTES: n/a PiperOrigin-RevId: 155913466 GitOrigin-RevId: 2d9cdee86aefb7dc3e777411eb075d07eb405a25 Change-Id: Ibae1e7bf2e7c327c39e8359c602e43146e42cae2 --- java/com/google/devtools/build/android/desugar/Desugar.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/com/google/devtools/build/android/desugar/Desugar.java b/java/com/google/devtools/build/android/desugar/Desugar.java index b4b4c8e..a09dc0d 100644 --- a/java/com/google/devtools/build/android/desugar/Desugar.java +++ b/java/com/google/devtools/build/android/desugar/Desugar.java @@ -550,7 +550,7 @@ class Desugar { private static Path createAndRegisterLambdaDumpDirectory() throws IOException { String propertyValue = System.getProperty(LambdaClassMaker.LAMBDA_METAFACTORY_DUMPER_PROPERTY); if (propertyValue != null) { - return Paths.get(propertyValue); + return Paths.get(propertyValue).toAbsolutePath(); } Path dumpDirectory = Files.createTempDirectory("lambdas"); -- cgit v1.2.3 From fdf79821aca7b0eff897f1037a9deae060113558 Mon Sep 17 00:00:00 2001 From: cnsun Date: Tue, 16 May 2017 18:01:18 +0200 Subject: Refine assertion by providing more information. When the given path is NOT a directory, output its path. RELNOTES: n/a PiperOrigin-RevId: 156187835 GitOrigin-RevId: 734d9e5663d86f9802c74a2ebd0e3ad9def0678e Change-Id: I4c38374b1e2ee86994496aa9fdd34786ca56034e --- java/com/google/devtools/build/android/desugar/LambdaClassMaker.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/com/google/devtools/build/android/desugar/LambdaClassMaker.java b/java/com/google/devtools/build/android/desugar/LambdaClassMaker.java index 155e323..beaeb03 100644 --- a/java/com/google/devtools/build/android/desugar/LambdaClassMaker.java +++ b/java/com/google/devtools/build/android/desugar/LambdaClassMaker.java @@ -35,7 +35,8 @@ class LambdaClassMaker { private final Map generatedClasses = new LinkedHashMap<>(); public LambdaClassMaker(Path rootDirectory) { - checkArgument(Files.isDirectory(rootDirectory)); + checkArgument( + Files.isDirectory(rootDirectory), "The argument '%s' is not a directory.", rootDirectory); this.rootDirectory = rootDirectory; } -- cgit v1.2.3 From 8625fbb68021ef98bd7b7ae6df995fde1a24b23b Mon Sep 17 00:00:00 2001 From: cnsun Date: Wed, 17 May 2017 03:03:46 +0200 Subject: Improve diagnostics. Report an error when we fail to register the lambda dump directory. This is achieved by checking InnerClassLambdaMetafactory.dumper's dumpDir. Also, if a user provides a dump folder, we verify whether the folder is empty. RELNOTES: n/a PiperOrigin-RevId: 156257767 GitOrigin-RevId: 69e855c7b0f0f7899a69a882cba0abd304233c97 Change-Id: I58e79e67c101b3dcf7992659cc0239ce0d656a8f --- .../devtools/build/android/desugar/Desugar.java | 47 ++++++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/java/com/google/devtools/build/android/desugar/Desugar.java b/java/com/google/devtools/build/android/desugar/Desugar.java index a09dc0d..10c2b99 100644 --- a/java/com/google/devtools/build/android/desugar/Desugar.java +++ b/java/com/google/devtools/build/android/desugar/Desugar.java @@ -16,6 +16,7 @@ package com.google.devtools.build.android.desugar; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.devtools.build.android.desugar.LambdaClassMaker.LAMBDA_METAFACTORY_DUMPER_PROPERTY; import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.auto.value.AutoValue; @@ -36,6 +37,7 @@ import com.google.errorprone.annotations.MustBeClosed; import java.io.IOError; import java.io.IOException; import java.io.InputStream; +import java.lang.reflect.Field; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; @@ -532,6 +534,8 @@ class Desugar { public static void main(String[] args) throws Exception { // It is important that this method is called first. See its javadoc. Path dumpDirectory = createAndRegisterLambdaDumpDirectory(); + verifyLambdaDumpDirectoryRegistered(dumpDirectory); + Options options = parseCommandLineOptions(args); if (options.verbose) { System.out.printf("Lambda classes will be written under %s%n", dumpDirectory); @@ -539,6 +543,29 @@ class Desugar { new Desugar(options, dumpDirectory).desugar(); } + static void verifyLambdaDumpDirectoryRegistered(Path dumpDirectory) { + try { + Class klass = Class.forName("java.lang.invoke.InnerClassLambdaMetafactory"); + Field dumperField = klass.getDeclaredField("dumper"); + dumperField.setAccessible(true); + Object dumperValue = dumperField.get(null); + checkNotNull(dumperValue, "Failed to register lambda dump directory '%s'", dumpDirectory); + + Field dumperPathField = dumperValue.getClass().getDeclaredField("dumpDir"); + dumperPathField.setAccessible(true); + Object dumperPath = dumperPathField.get(dumperValue); + checkState( + dumpDirectory.equals(dumperPath), + "Inconsistent lambda dump directories. real='%s', expected='%s'", + dumperPath, + dumpDirectory); + } catch (ReflectiveOperationException e) { + // We do not want to crash Desugar, if we cannot load or access these classes or fields. + // We aim to provide better diagnostics. If we cannot, just let it go. + e.printStackTrace(); + } + } + /** * LambdaClassMaker generates lambda classes for us, but it does so by essentially simulating the * call to LambdaMetafactory that the JVM would make when encountering an invokedynamic. @@ -547,15 +574,20 @@ class Desugar { * that case the specified directory is used as a temporary dir. Otherwise, it will be set here, * before doing anything else since the property is read in the static initializer. */ - private static Path createAndRegisterLambdaDumpDirectory() throws IOException { - String propertyValue = System.getProperty(LambdaClassMaker.LAMBDA_METAFACTORY_DUMPER_PROPERTY); + static Path createAndRegisterLambdaDumpDirectory() throws IOException { + String propertyValue = System.getProperty(LAMBDA_METAFACTORY_DUMPER_PROPERTY); if (propertyValue != null) { - return Paths.get(propertyValue).toAbsolutePath(); + Path path = Paths.get(propertyValue).toAbsolutePath(); + checkState(Files.isDirectory(path), "The path '%s' is not a directory.", path); + // It is not necessary to check whether 'path' is an empty directory. It is possible that + // LambdaMetafactory is loaded before this class, and there are already lambda classes dumped + // into the 'path' folder. + // TODO(kmb): Maybe we can empty the folder here. + return path; } Path dumpDirectory = Files.createTempDirectory("lambdas"); - System.setProperty( - LambdaClassMaker.LAMBDA_METAFACTORY_DUMPER_PROPERTY, dumpDirectory.toString()); + System.setProperty(LAMBDA_METAFACTORY_DUMPER_PROPERTY, dumpDirectory.toString()); deleteTreeOnExit(dumpDirectory); return dumpDirectory; } @@ -589,9 +621,8 @@ class Desugar { private static ImmutableList toInputOutputPairs(Options options) { final ImmutableList.Builder ioPairListbuilder = ImmutableList.builder(); for (Iterator inputIt = options.inputJars.iterator(), - outputIt = options.outputJars.iterator(); - inputIt.hasNext(); - ) { + outputIt = options.outputJars.iterator(); + inputIt.hasNext(); ) { ioPairListbuilder.add(InputOutputPair.create(inputIt.next(), outputIt.next())); } return ioPairListbuilder.build(); -- cgit v1.2.3