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