From db89a643180895c0b9ca27911adf1ff692212d1c Mon Sep 17 00:00:00 2001 From: Googler Date: Sun, 15 May 2016 18:30:52 +0000 Subject: Memory Optimizations: * Cache the FullyQualifiedName instances. Due to a very high number of duplicated resource keys, all FullyQualifiedNames should be effectively interned. * Presume xliff in all resources. Inlining the xmlns is a bit costly. -- MOS_MIGRATED_REVID=122375955 --- .../devtools/build/android/AndroidDataWriter.java | 5 +- .../devtools/build/android/FullyQualifiedName.java | 178 ++++++++++++--------- .../devtools/build/android/XmlResourceValues.java | 16 +- 3 files changed, 117 insertions(+), 82 deletions(-) (limited to 'src/tools/android/java/com/google/devtools/build') diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidDataWriter.java b/src/tools/android/java/com/google/devtools/build/android/AndroidDataWriter.java index 50895ac222..e3e7234f41 100644 --- a/src/tools/android/java/com/google/devtools/build/android/AndroidDataWriter.java +++ b/src/tools/android/java/com/google/devtools/build/android/AndroidDataWriter.java @@ -101,8 +101,9 @@ public class AndroidDataWriter implements Flushable, AndroidDataWritingVisitor { } } - private static final char[] START_RESOURCES = "".toCharArray(); - private static final char[] END_RESOURCES = "".toCharArray(); + public static final char[] START_RESOURCES = + ("").toCharArray(); + public static final char[] END_RESOURCES = "".toCharArray(); private static final char[] LINE_END = "\n".toCharArray(); private static final PngCruncher NOOP_CRUNCHER = new PngCruncher() { diff --git a/src/tools/android/java/com/google/devtools/build/android/FullyQualifiedName.java b/src/tools/android/java/com/google/devtools/build/android/FullyQualifiedName.java index cab92ff202..0f7ee755fc 100644 --- a/src/tools/android/java/com/google/devtools/build/android/FullyQualifiedName.java +++ b/src/tools/android/java/com/google/devtools/build/android/FullyQualifiedName.java @@ -18,7 +18,6 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; -import com.google.common.collect.Ordering; import com.google.devtools.build.android.proto.SerializeFormat; import com.android.resources.ResourceType; @@ -29,6 +28,10 @@ import java.nio.file.Paths; import java.util.Arrays; import java.util.List; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -46,89 +49,38 @@ public class FullyQualifiedName implements DataKey, Comparable qualifiers; - private final ResourceType resourceType; - private final String resourceName; - - /** - * Returns a string path representation of the FullyQualifiedName. - * - * Non-values Android Resource have a well defined file layout: From the resource directory, they - * reside in <resource type>[-<qualifier>]/<resource name>[.extension] - * - * @param sourceExtension The extension of the resource represented by the FullyQualifiedName - * @return A string representation of the FullyQualifiedName with the provided extension. - */ - public String toPathString(String sourceExtension) { - // TODO(corysmith): Does the extension belong in the FullyQualifiedName? - return Paths.get( - DASH_JOINER.join( - ImmutableList.builder() - .add(resourceType.getName()) - .addAll(qualifiers) - .build()), - resourceName + sourceExtension) - .toString(); - } - - @Override - public String toPrettyString() { - // TODO(corysmith): Add package when we start tracking it. - return String.format( - "%s/%s", - DASH_JOINER.join( - ImmutableList.builder().add(resourceType.getName()).addAll(qualifiers).build()), - resourceName); - } - - /** - * Returns the string path representation of the values directory and qualifiers. - * - * Certain resource types live in the "values" directory. This will calculate the directory and - * ensure the qualifiers are represented. - */ - // TODO(corysmith): Combine this with toPathString to clean up the interface of FullyQualifiedName - // logically, the FullyQualifiedName should just be able to provide the relative path string for - // the resource. - public String valuesPath() { - return Paths.get( - DASH_JOINER.join( - ImmutableList.builder().add("values").addAll(qualifiers).build()), - "values.xml") - .toString(); - } - - public String name() { - return resourceName; - } + // To save on memory, always return one instance for each FullyQualifiedName. + // Using a HashMap to deduplicate the instances -- the key retrieves a single instance. + private static final ConcurrentMap instanceCache = + new ConcurrentHashMap<>(); + private static final AtomicInteger cacheHit = new AtomicInteger(0); /** * A factory for parsing an generating FullyQualified names with qualifiers and package. */ public static class Factory { - + /** Used to adjust the api number for indvidual qualifiers. */ private static final class QualifierApiAdjuster { private final int minApi; private final ImmutableSet values; private final Pattern pattern; - + static QualifierApiAdjuster fromRegex(int minApi, String regex) { return new QualifierApiAdjuster(minApi, ImmutableSet.of(), Pattern.compile(regex)); } - + static QualifierApiAdjuster fromValues(int minApi, String... values) { return new QualifierApiAdjuster(minApi, ImmutableSet.copyOf(values), null); } - + private QualifierApiAdjuster( int minApi, @Nullable ImmutableSet values, @Nullable Pattern pattern) { this.minApi = minApi; this.values = ImmutableSet.copyOf(values); this.pattern = pattern; } - + /** Checks to see if the qualifier string is this type of qualifier. */ boolean check(String qualifier) { if (pattern != null) { @@ -136,13 +88,13 @@ public class FullyQualifiedName implements DataKey, Comparable minApi ? current : minApi; } } - + /** * An array used to calculate the api levels. * @@ -200,10 +152,10 @@ public class FullyQualifiedName implements DataKey, Comparable qualifiers; + private final ImmutableList qualifiers; private final String pkg; - private Factory(List qualifiers, String pkg) { + private Factory(ImmutableList qualifiers, String pkg) { this.qualifiers = qualifiers; this.pkg = pkg; } @@ -260,11 +212,11 @@ public class FullyQualifiedName implements DataKey, Comparable qualifiers, String pkg) { - return new Factory(qualifiers, pkg); + return new Factory(ImmutableList.copyOf(qualifiers), pkg); } public static Factory from(List qualifiers) { - return from(qualifiers, DEFAULT_PACKAGE); + return from(ImmutableList.copyOf(qualifiers), DEFAULT_PACKAGE); } public FullyQualifiedName create(ResourceType resourceType, String resourceName) { @@ -291,7 +243,7 @@ public class FullyQualifiedName implements DataKey, Comparable qualifiers, ResourceType resourceType, String resourceName) { - return new FullyQualifiedName( - pkg, Ordering.natural().immutableSortedCopy(qualifiers), resourceType, resourceName); + ImmutableList immutableQualifiers = ImmutableList.copyOf(qualifiers); + // TODO(corysmith): Address the GC thrash this creates by managing a simplified, mutable key to + // do the instance check. + FullyQualifiedName name = + new FullyQualifiedName(pkg, immutableQualifiers, resourceType, resourceName); + // Use putIfAbsent to get the canonical instance, if there. If it isn't, putIfAbsent will + // return null, and we should return the current instance. + FullyQualifiedName cached = instanceCache.putIfAbsent(name, name); + if (cached == null) { + return name; + } else { + cacheHit.incrementAndGet(); + return cached; + } } public static FullyQualifiedName fromProto(SerializeFormat.DataKey protoKey) { @@ -329,6 +293,71 @@ public class FullyQualifiedName implements DataKey, Comparable qualifiers; + private final ResourceType resourceType; + private final String resourceName; + + /** + * Returns a string path representation of the FullyQualifiedName. + * + * Non-values Android Resource have a well defined file layout: From the resource directory, they + * reside in <resource type>[-<qualifier>]/<resource name>[.extension] + * + * @param sourceExtension The extension of the resource represented by the FullyQualifiedName + * @return A string representation of the FullyQualifiedName with the provided extension. + */ + public String toPathString(String sourceExtension) { + // TODO(corysmith): Does the extension belong in the FullyQualifiedName? + return Paths.get( + DASH_JOINER.join( + ImmutableList.builder() + .add(resourceType.getName()) + .addAll(qualifiers) + .build()), + resourceName + sourceExtension) + .toString(); + } + + @Override + public String toPrettyString() { + // TODO(corysmith): Add package when we start tracking it. + return String.format( + "%s/%s", + DASH_JOINER.join( + ImmutableList.builder().add(resourceType.getName()).addAll(qualifiers).build()), + resourceName); + } + + /** + * Returns the string path representation of the values directory and qualifiers. + * + * Certain resource types live in the "values" directory. This will calculate the directory and + * ensure the qualifiers are represented. + */ + // TODO(corysmith): Combine this with toPathString to clean up the interface of FullyQualifiedName + // logically, the FullyQualifiedName should just be able to provide the relative path string for + // the resource. + public String valuesPath() { + return Paths.get( + DASH_JOINER.join( + ImmutableList.builder().add("values").addAll(qualifiers).build()), + "values.xml") + .toString(); + } + + public String name() { + return resourceName; + } + private FullyQualifiedName( String pkg, ImmutableList qualifiers, @@ -346,9 +375,7 @@ public class FullyQualifiedName implements DataKey, Comparable values = ImmutableMap.builder(); for (XMLEvent element = nextTag(eventReader); @@ -170,10 +174,14 @@ public class XmlResourceValues { } String value = escapeXmlValues(attribute.getValue()).replace("\"", """); if (!name.getNamespaceURI().isEmpty()) { - // Declare the xmlns here, so that the written xml will be semantically correct, - // if a bit verbose. This allows the resource keys to be written into a generic - // tag. - attributeMap.put("xmlns:" + name.getPrefix(), name.getNamespaceURI()); + // xliff is always provided on resources to help with file size + if (!(XLIFF_NAMESPACE.equals(name.getNamespaceURI()) + && XLIFF_PREFIX.equals(name.getPrefix()))) { + // Declare the xmlns here, so that the written xml will be semantically correct, + // if a bit verbose. This allows the resource keys to be written into a + // tag without a global namespace. + attributeMap.put("xmlns:" + name.getPrefix(), name.getNamespaceURI()); + } attributeMap.put(name.getPrefix() + ":" + attribute.getName().getLocalPart(), value); } else { attributeMap.put(attribute.getName().getLocalPart(), value); -- cgit v1.2.3