RFR [JEP 220] Modular Run-Time Images

Paul Sandoz paul.sandoz at oracle.com
Mon Dec 1 16:11:52 UTC 2014


On Nov 20, 2014, at 10:41 PM, Chris Hegarty <chris.hegarty at oracle.com> wrote:
>> 
>> Webrevs:
>>  http://cr.openjdk.java.net/~chegar/8061971/00/
>> 

Below is a review of JDK code. I did not spot any showstoppers. It's mostly cleanup related with maybe one or two bugs.

Paul.


In src/java.base/share/classes/java/util/jar/Attributes.java
        /**
          * <code>Name</code> object for <code>Implementation-URL</code>
          * manifest attribute used for package versioning.
-         * @see <a href="../../../../technotes/guides/versioning/spec/versioning2.html#wp90779">
-         *      Java Product Versioning Specification</a>
+         *
+         * @deprecated Extension mechanism is no longer supported.
          */
+        @Deprecated
         public static final Name IMPLEMENTATION_URL = new Name("Implementation-URL");
Is that is marked as deprecated should all the other IMPLEMENTATION_* names be marked as deprecated?


In src/java.base/share/classes/sun/misc/Launcher.java
+        private static URL[] getExtURLs(File[] files) throws IOException {
+            List<URL> urls = new ArrayList<>();
+            for (File f : files) {
+                urls.add(getFileURL(f));
             }
-            return null;
+            return urls.toArray(new URL[0]);
         }
Not important, but you can just create the URL array of the correct size rather than going through an ArrayList.


In make/src/classes/build/tools/module/ImageBuilder.java, some stream usages, take 'em or leave 'em:

+    class SimpleResolver {
+        private final Set<Module> initialMods;
+        private final Map<String,Module> nameToModule = new HashMap<>();
+
+        SimpleResolver(Set<String> mods, Set<Module> graph) {
+            graph.stream()
+                 .forEach(m -> nameToModule.put(m.name(), m));
+            initialMods = mods.stream()
+                         .map(this::nameToModule)
+                         .collect(Collectors.toSet());
+        }

You might be able to use toMap to create a map:

  nameToModule = graph.stream().collect(toMap(Module::name, m -> m));

That will throw an error on duplicate keys, but you can use the three-arg form to override that.

+        private Set<Module> getModuleDependences(Module m) {
+            return m.requires().stream()
+                    .map(d -> d.name())
+                    .map(this::nameToModule)
+                    .collect(Collectors.toSet());
+        }
You can return a stream, change "collect" to "distinct", then you don't need to stream over the set again when using forEach (but the exception code needs to be updated to collect as a set)


+        private Path moduleToPath(String name, List<Path> paths, boolean expect) {
+            Set<Path> foundPaths = new HashSet<>();
+            if (paths != null) {
+                for (Path p : paths) {
+                    Path rp = p.resolve(name);
+                    if (Files.exists(rp))
+                        foundPaths.add(rp);
+                }
+            }
+            if (foundPaths.size() > 1)
+                throw new RuntimeException("Found more that one path for " + name);
+            if (expect && foundPaths.size() != 1)
+                throw new RuntimeException("Expected to find classes path for " + name);
+            return foundPaths.size() == 0 ? null : foundPaths.iterator().next();
+        }

  // Stop when there is more than 1
  Set<Path> foundPaths = paths == null ? Collections.emptySet() : paths.stream().map(p -> p.resolve(name)).filter(Files::exists).limit(2).collect(toSet());


+    private Option getOption(String name) throws BadArgs {
+        for (Option o : recognizedOptions) {
+            if (o.matches(name)) {
+                return o;
+            }
+        }
+        throw new BadArgs("Unknown option %s", name).showUsage(true);
+    }
  Stream.of(recognizedOptions).filter(o -> o.matches(name)).findFirst().orElseThrow(() -> new BadArgs("Unknown option %s", name).showUsage(true));


In src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java

+    public String[] getEntryNames(boolean sorted) {
+        int count = header.getLocationCount();
+        List<String> list = new ArrayList<>();
+
+        for (int i = 0; i < count; i++) {
+            int offset = getOffset(i);
+
+            if (offset != 0) {
+                ImageLocation location = ImageLocation.readFrom(locationsBuffer, offset, strings);
+                list.add(location.getFullnameString());
+            }
+        }
+
+        String[] array = list.toArray(new String[0]);
+
+        if (sorted) {
+            Arrays.sort(array);
+        }
+
+        return array;
+    }

  Stream<String> names = IntStream.range(0, count).map(offsetsBuffer::get).filter(o -> o != 0).mapToObj(...);
  if (sorted) names = names.sorted();
  return names.toArray(String[]::new);


+    protected ImageLocation[] getAllLocations(boolean sorted) {
+        int count = header.getLocationCount();
+        List<ImageLocation> list = new ArrayList<>();
+
+        for (int i = 0; i < count; i++) {
+            int offset = getOffset(i);
+
+            if (offset != 0) {
+                ImageLocation location = ImageLocation.readFrom(locationsBuffer, offset, strings);
+                list.add(location);
+            }
+        }
+
+        ImageLocation[] array = list.toArray(new ImageLocation[0]);
+
+        if (sorted) {
+            Arrays.sort(array, (ImageLocation loc1, ImageLocation loc2) ->
+                    loc1.getFullnameString().compareTo(loc2.getFullnameString()));
+        }
+
+        return array;
+    }

  Stream<ImageLocation> locations = IntStream.range(0, count).map(offsetsBuffer::get).filter(o -> o != 0).mapToObj(...);
  if (sorted) locations = locations.sorted(Comparing.comparing(ImageLocation::getFullnameString));
  return locations.toArray(ImageLocation[]::new);


In src/java.base/share/classes/jdk/internal/jimage/BasicImageWriter.java

+    private ImageBucket[] createBuckets() {
+        ImageBucket[] buckets = new ImageBucket[count];
+
+        input.stream().forEach((location) -> {
+            int index = location.hashCode() % count;
+            ImageBucket bucket = buckets[index];
+
+            if (bucket == null) {
+                buckets[index] = bucket = new ImageBucket();
+            }
+
+            bucket.add(location);
+        });
+
+        ImageBucket[] sorted = Arrays.asList(buckets).stream()
+                .filter((bucket) -> (bucket != null))
+                .sorted()
+                .toArray(ImageBucket[]::new);
+
+        return sorted;
+    }
Use Stream.of(buckets).filter(....).

In src/java.base/share/classes/jdk/internal/jimage/ImageFile.java

+    private void readModuleEntries(ImageModules modules,
+                                   Set<Archive> archives)
+        throws IOException
+    {
+        for (Archive archive : archives) {
+            List<Resource> res = new ArrayList<>();
+            archive.visitResources(x-> res.add(x));
+
res::add
+            String mn = archive.moduleName();
+            resourcesForModule.put(mn, res);
+
+            Set<String> pkgs = res.stream().map(Resource::name)
+                    .filter(n -> n.endsWith(".class"))
+                    .map(this::toPackage)
+                    .distinct()
+                    .collect(Collectors.toSet());
+            modules.setPackages(mn, pkgs);
+        }
+    }

"distinct" is not required since the values are collected into a set.


+    static class Compressor {
+        public static byte[] compress(byte[] bytesIn) {
+            Deflater deflater = new Deflater();
+            deflater.setInput(bytesIn);
+            ByteArrayOutputStream stream = new ByteArrayOutputStream(bytesIn.length);
+            byte[] buffer = new byte[1024];
+
+            deflater.finish();
+            while (!deflater.finished()) {
+                int count = deflater.deflate(buffer);
+                stream.write(buffer, 0, count);
+            }
+
+            try {
+                stream.close();
+            } catch (IOException ex) {
+                return bytesIn;
+            }
+
+            byte[] bytesOut = stream.toByteArray();
+            deflater.end();
+
+            return bytesOut;
+        }

ByteArrayOutputStream.close does not need to be called, it's a nop. Same applies for the decompress method that follows the above method.


In src/java.base/share/classes/jdk/internal/jimage/ImageModules.java

+    private void mapModulesToLoader(Loader loader, Set<String> modules) {
+        if (modules.isEmpty())
+            return;
+
+        // put java.base first
+        Set<String> mods = new LinkedHashSet<>();
+        modules.stream()
+               .filter(m -> m.equals("java.base"))
+               .forEach(mods::add);
+        modules.stream().sorted()
+               .filter(m -> !m.equals("java.base"))
+               .forEach(mods::add);
+        loaders.put(loader, new LoaderModuleData(loader, mods));
+    }

Since the second pass sorts it should be possible to avoid the first pass by using a comparator that sorts java.base before all other names.

  Comparator<String> cs = (a, b) -> {
    
    boolean ajb = a.equals("java.base");
    
    boolean bjb = b.equals("java.base");
    
    return (ajb && bjb) ? 0 : ajb ? -1 : bjb ? 1 : a.compareTo(b);

  };

  Set<String> mods = modules.stream().sorted(cs).collect(toCollection(LinkedHashSet::new));


+    public class ModuleIndex {
+        final Map<String, Integer> moduleOffsets = new LinkedHashMap<>();
+        final Map<String, List<Integer>> packageOffsets = new HashMap<>();
+        final int size;

Might want to consider changing List<Integer> to int[]. The two maps could be unified to just one Map<String, int[]> since the same key is used, and the first element in the array corresponds to the module name string index.


In src/java.base/share/classes/jdk/internal/jimage/ImageReader.java
+    private void fillPackageModuleInfo() {
+        assert rootDir != null;
+
+        packageMap.entrySet().stream().sorted((x, y)->x.getKey().compareTo(y.getKey())).forEach((entry) -> {

Use Map.Entry.comparingByKey().


In src/java.base/share/classes/jdk/internal/jrtfs/JrtDirectoryStream.java

+final class JrtDirectoryStream implements DirectoryStream<Path> {
+    private final JrtFileSystem jrtfs;
+    private final byte[] path;
+    private final DirectoryStream.Filter<? super Path> filter;
+    private volatile boolean isClosed;
+    private volatile Iterator<Path> itr;
Why are itr and isClosed fields marked volatile when the iterator() and closed() methods are synchronized?

+        return new Iterator<Path>() {
+            private Path next;
Field not used.

+            @Override
+            public boolean hasNext() {
+                if (isClosed)
+                    return false;
+                return itr.hasNext();
+            }
+
+            @Override
+            public synchronized Path next() {
+                if (isClosed)
+                    throw new NoSuchElementException();
+                return itr.next();
+            }
+
+            @Override
+            public void remove() {
+                throw new UnsupportedOperationException();
+            }
Do not need to implemented "remove" now it is defined as a default method.

In src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java
+    @Override
+    public Iterable<Path> getRootDirectories() {
+        ArrayList<Path> pathArr = new ArrayList<>();
+        pathArr.add(rootPath);
+        return pathArr;
+    }

Can use Arrays.asList(rootPath) since it is unspecified what happens if iterator().remove() is called? same for implementation of getFileStores.


+    private Iterator<Path> nodesToIterator(Path path, List<Node> childNodes) {
+        List<Path> childPaths = new ArrayList<>(childNodes.size());
+        childNodes.stream().forEach((child) -> {
+            childPaths.add(toJrtPath(child.getNameString()));
+        });
+        return childPaths.iterator();
+    }

Assuming that removal is not supported and you are allowed to be lazy over the contents of childNodes:

  childNodes.stream().map(c -> toJrtPath(c.getNameString())).iterator();

Otherwise if you need to make a copy:

 childNodes.stream().map(c -> toJrtPath(c.getNameString())).collect(toList()).iterator();


In src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystemProvider.java
+    private FileSystem getTheFileSystem() {
+        FileSystem fs = this.theFileSystem;
+        if (fs == null) {
+            synchronized (this) {
+                fs = this.theFileSystem;
+                if (fs == null) {
+                    try {
+                        fs = new JrtFileSystem(this, null) {
+                            @Override public void close() {
+                                throw new UnsupportedOperationException();
+                            }
+                        };
+                    } catch (IOException ioe) {
+                        throw new InternalError(ioe);
+                    }
+                }
+                this.theFileSystem = fs;
+            }
+        }
+        return fs;
+    }

More readable if you do:

  this.fileSystem = fs = ...;

rather than have a potentially redundant volatile store.


In src/java.base/share/classes/jdk/internal/jrtfs/JrtPath.java

+final class JrtPath implements Path {
+
+    private final JrtFileSystem jrtfs;
+    private final byte[] path;
+    private volatile int[] offsets;

Some of the code in JrtPath is gonna take a hit on repeated volatile loads of "offsets" so recommend reading it once to a local variable. Perhaps  initOffset can return the offset array so one can easily use it as a local variable:

  int[] localOffsets = initOffsets();

You might want to document that  equalsNameAt does not need to initialize the offsets of this and the other path and perhaps add some asserts.


+    // resolved path for locating jrt entry inside the jrt file,
+    // the result path does not contain ./ and .. components
+    private volatile byte[] resolved = null;
+    byte[] getResolvedPath() {
+        byte[] r = resolved;
+        if (r == null) {
+            if (isAbsolute())
+                r = getResolved();
+            else
+                r = toAbsolutePath().getResolvedPath();
+            if (r.length > 0 && r[0] == '/')
+                r = Arrays.copyOfRange(r, 1, r.length);
+            resolved = r;
+        }
+        return resolved;
+    }

Return "r" instead of "resolved".


In src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
+    @Override
+    public int getContentLength() {
+        long len = getContentLengthLong();
+        return Math.min((int)len, Integer.MAX_VALUE);
+    }
That is not correct as the narrowing conversion will just discard the higher order bits (e.g. Math.min((int)(1L << 32), Integer.MAX_VALUE) ). I think you need to do:

  return len > Integer.MAX_VALUE ? -1 : (int)len;
  

In src/jdk.dev/share/classes/jdk/tools/jimage/JImageTask.java

In the method "recreate" a Stream.reduce function is used over files in the image. This reduction returns the total size but also performs some side-effects and supplies a bogus combiner since the total result is never used. I think it would be better to use a peek to perform some side-effects then you can consume that stream to create a new image and there is no need to create a List<File>.

The code here:
+                                Stream<Integer> offsets = Files.readAllLines(path)
+                                        .stream()
+                                        .map(writer::addString);
+                                size = offsets.count() * 4;
Can be replaced with:

  // The peek op has side-effects
  size = Files.lines(path).peek(s -> writer.addString(s)).count() * 4;


The code here:

+                            List<Integer> offsets = Files.readAllLines(path)
+                                    .stream()
+                                    .map(writer::addString)
+                                    .collect(Collectors.toList());
+                            for (int off : offsets) {
+                                out.writeInt(off);
+                            }
Can be replaced with:

  // The mapToInt op has side-effects
  Files.lines(path).mapToInt(writer::addString).forEach(out::writeInt);

Paul.


More information about the jdk9-dev mailing list