JDK-6435956 : javac DefaultFileManager.Archive retains excessive memory for Zip entry names
  • Type: Enhancement
  • Component: tools
  • Sub-Component: javac
  • Affected Version: 6
  • Priority: P4
  • Status: Closed
  • Resolution: Won't Fix
  • OS: generic
  • CPU: generic
  • Submitted: 2006-06-08
  • Updated: 2015-11-17
  • Resolved: 2015-11-17
Related Reports
Relates :  
Description
A DESCRIPTION OF THE FIX :
javac DefaultFileManager.Archive retains excessive memory for Zip entry names

For each additional entry within a "directory" of a zip/jar, the current implementation retains a List, a String and a long char[]. The char[] is longer than necessary because the String created by String.substring contains the entire path name, As well as increasing the amount of memory necessary, the extra objects may impact GC performance for longer lived processes.

The patch replaces the oversized String with a trimmed char[]. The custom linked List implementation is replaced by a standard ArrayList. So for each additional entry we have one short char[] and one or two extra elements in the ArrayList array.

The patch does increase start up times marginally because of the extra copying. However, javac becomes more likely to complete.

As an example, without the patch the following command gives an OutOfMemoryError dump (assuming the mustang source is in the specified directory).

javac -d scratch/  $( find ~/mustang/current/j2se/src/share/classes/java/ -name "*.java" -print | grep --invert-match --regexp=- )



--- /home/tackline/mustang/current/j2se/src/share/classes/com/sun/tools/javac/util/DefaultFileManager.java      2006-05-26 12:01:51.000000000 +0100
+++ java/com/sun/tools/javac/util/DefaultFileManager.java       2006-05-27 12:47:51.000000000 +0100
@@ -269,10 +269,10 @@
                 subdirectory = subdirectory.replace('\\', '/');
                 if (!subdirectory.endsWith("/")) subdirectory = subdirectory + "/";
             }
-            List<String> files = archive.map.get(subdirectory);
+            ArrayList<char[]> files = archive.map.get(subdirectory);
             if (files != null) {
-                for (String file; !files.isEmpty(); files = files.tail) {
-                    file = files.head;
+                for (char[] fileChars : files) {
+                    String file = String.valueOf(fileChars);
                     if (isValidFile(file, fileKinds)) {
                         ZipEntry ze = archive.zdir.getEntry(subdirectory + file);
                         JavaFileObject fe = new ZipFileObject(file, archive.zdir, ze);
@@ -376,9 +376,9 @@
      * mapping directory names to lists of files (basenames).
      */
     public static class Archive {
-        final Map<String,List<String>> map;
+        final Map<String,ArrayList<char[]>> map;
         final public ZipFile zdir;
-        public Archive(ZipFile zdir, Map<String,List<String>> map) {
+        public Archive(ZipFile zdir, Map<String,ArrayList<char[]>> map) {
             this.zdir = zdir;
             this.map = map;
         }
@@ -389,8 +389,16 @@
             String basename = name.substring(i+1);
             if (basename.length() == 0)
                 return false;
-            List<String> list = map.get(dirname);
-            return (list != null && list.contains(basename));
+            ArrayList<char[]> list = map.get(dirname);
+            if (list == null)
+                return false;
+            char[] basenameChars = basename.toCharArray();
+            for (char[] candidate : list) {
+                if (java.util.Arrays.equals(basenameChars, candidate)) {
+                    return true;
+                }
+            }
+            return false;
         }
     }
     public static class MissingArchive extends Archive {
@@ -422,8 +430,8 @@
                 return archive;
             }

-            final Map<String,List<String>> map
-                = new HashMap<String,List<String>>();
+            final Map<String,ArrayList<char[]>> map
+                = new HashMap<String,ArrayList<char[]>>();

             for (java.util.Enumeration<? extends ZipEntry> e = zdir.entries();
                  e.hasMoreElements();) {
@@ -431,14 +439,15 @@
                 String name = entry.getName();
                 int i = name.lastIndexOf('/');
                 String dirname = name.substring(0, i+1);
-                String basename = name.substring(i+1);
-                if (basename.length() == 0)
+                char[] basename = name.substring(i+1).toCharArray();
+                if (basename.length == 0)
                     continue;
-                List<String> list = map.get(dirname);
-                if (list == null)
-                    list = List.nil();
-                list = list.prepend(basename);
-                map.put(dirname, list);
+                ArrayList<char[]> list = map.get(dirname);
+                if (list == null) {
+                    list = new ArrayList<char[]>();
+                    map.put(new String(dirname), list);
+                }
+                list.add(basename);
             }

             archive = new Archive(zdir, map);

Comments
No longer relevant as we transition to a Path based file manager
17-11-2015

Deferred to 9.
14-06-2013

EVALUATION Would be better to consider more substantial overhaul, such as coming up with structured names rather than simply changing Striing to char[], since so much of the duplication is in long directory names shared between classes.
13-01-2011

EVALUATION http://blogs.sun.com/ahe?entry=contributing_to_javac
08-06-2006

EVALUATION This raises a number of good points: * com.sun.tools.javac.util.List is not the best data structure here. * The strings should be pruned (we could save even more space by using com.sun.tools.javac.util.Name). However, this also affects the startup time and this is a big concern for us because of how it affects when javac is used in more interactive environments such as in an IDE, Ant, or in a JSP server. Due to JSR 199 work there is still a few changes remaining in DefaultFileManager. This may have to wait to a Mustang update release. A less disruptive short term solution we may consider is this: * Make sure to use new String(xyz.substring(...)) * Switch to ArrayList We have a few ideas for improving the Name class that would make it even better in this case. However, this is a speed vs footprint trade off, so perhaps it should be configurable.
08-06-2006