[gwt-contrib] Using cached ZipFileClassPathEntry objects. (issue1388803)
Reviewers: conroy,
Description:
Using cached ZipFileClassPathEntry objects.
While this does not give benefits for DevMode, which parses jar files
only once, GWT Designer does this many times.
This gives about 15% speed up in GWT Designer.
Initial.
-
Parsing...done: 4775
refresh: 296
palette: 114
Parsing...done: 2445
refresh: 274
palette: 38
Parsing...done: 2396
refresh: 272
palette: 37
Cache ZipFileClassPathEntry
-
Parsing...done: 4299
refresh: 304
palette: 116
Parsing...done: 2161
refresh: 277
palette: 41
Parsing...done: 2087
refresh: 270
palette: 38
Please review this at http://gwt-code-reviews.appspot.com/1388803/
Affected files:
M dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java
M dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java
M
dev/core/test/com/google/gwt/dev/resource/impl/AbstractResourceOrientedTestBase.java
M
dev/core/test/com/google/gwt/dev/resource/impl/ResourceOracleImplTest.java
Index: dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java
===
--- dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java
(revision 9867)
+++ dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java
(working copy)
@@ -156,14 +156,14 @@
if (f.isDirectory()) {
return new DirectoryClassPathEntry(f);
} else if (f.isFile() && lowerCaseFileName.endsWith(".jar")) {
-return new ZipFileClassPathEntry(f);
+return ZipFileClassPathEntry.get(f);
} else if (f.isFile() && lowerCaseFileName.endsWith(".zip")) {
-return new ZipFileClassPathEntry(f);
+return ZipFileClassPathEntry.get(f);
} else {
// It's a file ending in neither jar nor zip, speculatively try to
// open as jar/zip anyway.
try {
- return new ZipFileClassPathEntry(f);
+ return ZipFileClassPathEntry.get(f);
} catch (Exception ignored) {
}
logger.log(TreeLogger.TRACE, "Unexpected entry in classpath; " + f
Index:
dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java
===
---
dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java
(revision 9867)
+++
dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java
(working copy)
@@ -21,6 +21,9 @@
import com.google.gwt.dev.util.collect.IdentityMaps;
import com.google.gwt.dev.util.collect.Sets;
import com.google.gwt.dev.util.msg.Message1String;
+
+import org.apache.commons.collections.map.AbstractReferenceMap;
+import org.apache.commons.collections.map.ReferenceMap;
import java.io.File;
import java.io.IOException;
@@ -65,6 +68,37 @@
this.cachedAnswers = cachedAnswers;
}
}
+
+ private static class ZipFileCachedEntry {
+private final long lastModified;
+private final ZipFileClassPathEntry entry;
+public ZipFileCachedEntry(File zipFile, ZipFileClassPathEntry entry) {
+ this.lastModified = zipFile.lastModified();
+ this.entry = entry;
+}
+public boolean isStale(File zipFile) {
+ return lastModified != zipFile.lastModified();
+}
+ }
+
+ @SuppressWarnings("unchecked")
+ private static final Map entryCache = new
ReferenceMap(
+ AbstractReferenceMap.HARD, AbstractReferenceMap.SOFT);
+
+ /**
+ * @return the {@link ZipFileClassPathEntry} instance for given jar or
zip
+ * file, may be shared with other users.
+ */
+ public static ZipFileClassPathEntry get(File zipFile) throws IOException
{
+String location = zipFile.toURI().toString();
+ZipFileCachedEntry cachedEntry = entryCache.get(location);
+if (cachedEntry == null || cachedEntry.isStale(zipFile)) {
+ ZipFileClassPathEntry entry = new ZipFileClassPathEntry(zipFile);
+ cachedEntry = new ZipFileCachedEntry(zipFile, entry);
+ entryCache.put(location, cachedEntry);
+}
+return cachedEntry.entry;
+ }
private Set allZipFileResources;
@@ -77,7 +111,7 @@
private final ZipFile zipFile;
- public ZipFileClassPathEntry(File zipFile) throws IOException {
+ private ZipFileClassPathEntry(File zipFile) throws IOException {
assert zipFile.isAbsolute();
this.zipFile = new ZipFile(zipFile);
this.location = zipFile.toURI().toString();
Index:
dev/core/test/com/google/gwt/dev/resource/impl/AbstractResourceOrientedTestBase.java
===
---
dev/core/test/com/google/gwt/dev/resource/impl/AbstractResourceOrientedTestBase.java
(revision 9867)
+++
dev/core/test/com/google/gwt/dev/resource/impl/AbstractResourceOrientedTestBase.java
(working copy)
@@ -193,12 +193,12 @@
protected ClassPathEntry getClassPathEntry1AsJar() throws IOException,
URISyntaxException {
File file =
findFile("com/google/gwt/dev/resource/impl/t
[gwt-contrib] Re: Fix Javadoc for gesture/touch events (issue1383805)
On 2011/03/20 04:45:50, fredsa wrote: LGTM http://gwt-code-reviews.appspot.com/1383805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using cached ZipFileClassPathEntry objects. (issue1388803)
http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java File dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java (right): http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode85 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:85: private static final Map entryCache = new ReferenceMap( Why choose the apache commons library over WeakHashMap (JRE)? http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode98 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:98: entryCache.put(location, cachedEntry); Any idea how much additional memory this takes? http://gwt-code-reviews.appspot.com/1388803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using cached ZipFileClassPathEntry objects. (issue1388803)
What is the use case here? Normally, a jar file is never re-opened in a java classpath after the first time it is loaded, so I'm not sure I understand what's happening here. Is GWTDesigner generating and overwriting jar files and then reloading them? How has this worked in the past, given that ResourceOracle doesn't reload jar file entries after the first time. In otherwords, I'm not sure I understand the delta here from how it was working before. Also, it looks like it is changing the behavior of ResourceOracle, since it will now be sensitive to changes in a jar file (whereas it has not been in the past). http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java File dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java (right): http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode98 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:98: entryCache.put(location, cachedEntry); It would seem this shouldn't take up more memory than the current implementation, if I understand correctly? It looks like it's keeping track of all ZipFileClassPathEntries and allocating replaced versions if they become stale. I guess the memory hit would be if multiple versions of the same jar file are referenced?). What about multiple threads, does this master entryCache need any synchronization? http://gwt-code-reviews.appspot.com/1388803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
(right):
http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode353
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:353:
synchronized (unitMap) {
According to the javadoc for Collections.synchronizedMap(): "It is
imperative that the user manually synchronize on the returned map when
iterating over any of its collection views"(I think Eric added the
synchronization here in response to an earlier comment of mine). I
think this is correct here, and in this case we do want to lock the
queue while cleaning up, no? (For ordering/precedence reasons?).
http://gwt-code-reviews.appspot.com/1375802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using cached ZipFileClassPathEntry objects. (issue1388803)
DevMode uses single top level ModuleDef with single set of jars. GWT Designer can open several editors with several top level ModuleDefs, which use several may be different sets of jars, but with many intersections. If only we could have hashCode() and equals() for PathPrefixSet... This would allow not only cashing and reusing indexed jars, but also results of resource requests - another 15% of performance... http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java File dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java (right): http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode85 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:85: private static final Map entryCache = new ReferenceMap( WeakHashMap is for weak reference on key. SoftReference is for memory-sensitive cache. http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode98 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:98: entryCache.put(location, cachedEntry); In reality memory consumption is lower with caching. We don't generate new ZipFileClassPathEntry with all its ZipResources. With cache: 14 instances, 8.5MB Without cache: 30 instances, 29MB Cache prevents creating several instances for same jar. Keep in mind difference of DevMove and DesignMode. First uses fixed set of jars in classpath. Second uses several set of jars, possibly different in different editors, but often there are same jars. Most prominent of these "same jars" in JRE jars (which may be should not be indexed at all) and gwt-user.jar if all GWT modules of user use same version of GWT. Check isStale() is mostly paranoidal - what if user closed all its GWT Designer editors and replaced some jar with newer version, but using same name. This almost never happens, but it is easy and cheap to implement such check. It is not possible to have several versions of same jar - if they have same URI, then old one was replaced with newer. You are right about synchronization. It is required for entryCache and also for findApplicableResources(). http://gwt-code-reviews.appspot.com/1388803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using cached ZipFileClassPathEntry objects. (issue1388803)
http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java File dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java (right): http://gwt-code-reviews.appspot.com/1388803/diff/1/dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java#newcode98 dev/core/src/com/google/gwt/dev/resource/impl/ZipFileClassPathEntry.java:98: entryCache.put(location, cachedEntry); I think we should remove the isStale check, since this violates the current semantics for jar files in DevMode (and for jar files in a java classpath). Once a jar file is opened and indexed, it is not re-indexed if someone comes in and updates that jar file. http://gwt-code-reviews.appspot.com/1388803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
