Adjusting tests and fixing loose ends from merge.

Author: david-beaumont

src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

@@ -46,7 +46,6 @@
 import java.util.TreeMap;
 import java.util.function.Function;
 import java.util.function.Supplier;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static jdk.internal.jimage.ImageLocation.FLAGS_HAS_PREVIEW_VERSION;
@@ -337,15 +336,12 @@ private ArrayList<Node> processPackagesDirectory(boolean previewMode) {
                 if (!previewMode || (flags & FLAGS_HAS_PREVIEW_VERSION) == 0) {
                     continue;
                 }
-                // Only do this in preview mode for the small set of packages
-                // with preview versions (these offset buffers should be small).
-                List<ModuleReference> modRefs =
-                        ModuleReference.read(getOffsetBuffer(pkgDir), true, this::getString);
-                List<String> previewModuleNames = modRefs.stream()
-                        .filter(ModuleReference::hasPreviewVersion)
-                        .map(ModuleReference::name)
-                        .collect(Collectors.toList());
-                previewPackagesToModules.put(pkgDir.getBase().replace('.', '/'), previewModuleNames);
+                // Only do this in preview mode for the small set of packages with
+                // preview versions (the number of preview entries should be small).
+                List<String> moduleNames = new ArrayList<>();
+                ModuleReference.readNameOffsets(getOffsetBuffer(pkgDir), /*normal*/ false, /*preview*/ true)
+                        .forEachRemaining(n -> moduleNames.add(getString(n)));
+                previewPackagesToModules.put(pkgDir.getBase().replace('.', '/'), moduleNames);
             }
             // Reverse sorted map means child directories are processed first.
             previewPackagesToModules.forEach((pkgPath, modules) ->
@@ -489,7 +485,7 @@ Node findResourceNode(String moduleName, String resourcePath) {
             if (moduleName.indexOf('/') >= 0) {
                 throw new IllegalArgumentException("invalid module name: " + moduleName);
             }
-            String nodeName = MODULES_ROOT + "/" + moduleName + "/" + resourcePath;
+            String nodeName = MODULES_PREFIX + "/" + moduleName + "/" + resourcePath;
             // Synchronize as tightly as possible to reduce locking contention.
             synchronized (this) {
                 Node node = nodes.get(nodeName);
@@ -523,7 +519,7 @@ boolean containsResource(String moduleName, String resourcePath) {
             // In preview mode, preview-only resources are eagerly added to the
             // cache, so we must check that first.
             if (previewMode) {
-                String nodeName = MODULES_ROOT + "/" + moduleName + "/" + resourcePath;
+                String nodeName = MODULES_PREFIX + "/" + moduleName + "/" + resourcePath;
                 // Synchronize as tightly as possible to reduce locking contention.
                 synchronized (this) {
                     Node node = nodes.get(nodeName);
@@ -612,8 +608,10 @@ private Node buildAndCacheLinkNode(String name) {
                 // If no parent exists here, the name cannot be valid.
                 Directory parent = (Directory) nodes.get(dirName);
                 if (parent != null) {
-                    // This caches all child links of the parent directory.
-                    completePackageSubdirectory(parent, findLocation(dirName));
+                    if (!parent.isCompleted()) {
+                        // This caches all child links of the parent directory.
+                        completePackageSubdirectory(parent, findLocation(dirName));
+                    }
                     return nodes.get(name);
                 }
             }
@@ -665,25 +663,22 @@ private Directory completeModuleDirectory(Directory dir, ImageLocation loc) {
             return dir;
         }
 
-        /**
-         * Completes a package directory by setting the list of child nodes.
-         *
-         * <p>The given directory can be the top level {@code /packages} directory,
-         * so it is NOT safe to use {@code isPackagesSubdirectory(loc)} here.
-         */
+        /** Completes a package directory by setting the list of child nodes. */
         private void completePackageSubdirectory(Directory dir, ImageLocation loc) {
             assert dir.getName().equals(loc.getFullName()) : "Mismatched location for directory: " + dir;
-            LocationType type = loc.getType();
-            assert type == PACKAGES_DIR : "Invalid location type: " + loc;
-
-            List<ModuleReference> modRefs =
-                    ModuleReference.read(getOffsetBuffer(loc), previewMode, this::getString);
-            List<Node> children = modRefs.stream()
-                    .map(mod -> ensureCached(
-                            newLinkNode(
-                                    dir.getName() + "/" + mod.name(),
-                                    MODULES_PREFIX + "/" + mod.name())))
-                    .collect(Collectors.toList());
+            assert !dir.isCompleted() : "Directory already completed: " + dir;
+            assert loc.getType() == PACKAGES_DIR : "Invalid location type: " + loc.getType();
+
+            // In non-preview mode we might skip a very small number of preview-only
+            // entries, but it's not worth "right-sizing" the array for that.
+            IntBuffer offsets = getOffsetBuffer(loc);
+            List<Node> children = new ArrayList<>(offsets.capacity() / 2);
+            ModuleReference.readNameOffsets(offsets, /*normal*/ true, previewMode)
+                    .forEachRemaining(n -> {
+                        String modName = getString(n);
+                        Node link = newLinkNode(dir.getName() + "/" + modName, MODULES_PREFIX + "/" + modName);
+                        children.add(ensureCached(link));
+                    });
             // If the parent directory exists, there must be at least one child node.
             assert !children.isEmpty() : "Invalid empty package directory: " + dir;
             dir.setChildren(children);

src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java

@@ -28,9 +28,16 @@
 import java.nio.IntBuffer;
 import java.util.ArrayList;
 import java.util.Comparator;
+import java.util.Iterator;
 import java.util.List;
+import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.function.Function;
+import java.util.function.IntFunction;
+import java.util.function.IntPredicate;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+import java.util.stream.Stream;
 
 /**
  * Represents the module entries stored in the buffer of {@code "/packages/xxx"}
@@ -59,11 +66,12 @@ public final class ModuleReference implements Comparable<ModuleReference> {
     private static final int FLAGS_HAS_PREVIEW_VERSION = 0x4;
 
     /**
-     * References are ordered so the unique module in which the associated
-     * package is non-empty comes first.
+     * References are ordered with preview versions first which permits early
+     * exit when processing preview entries (it's reversed because the default
+     * order for a boolean is {@code false < true}).
      */
-    private static final Comparator<ModuleReference> NON_EMPTY_FIRST =
-            Comparator.comparing(ModuleReference::isEmpty)
+    private static final Comparator<ModuleReference> PREVIEW_FIRST =
+            Comparator.comparing(ModuleReference::hasPreviewVersion).reversed()
                     .thenComparing(ModuleReference::name);
 
     /** Creates a reference for an empty package (one without content in). */
@@ -109,8 +117,8 @@ public String name() {
      * <p>An invariant of the module system is that while a package may exist
      * under many modules, it is only non-empty in one.
      */
-    public boolean isEmpty() {
-        return ((flags & FLAGS_HAS_CONTENT) == 0);
+    public boolean hasContent() {
+        return ((flags & FLAGS_HAS_CONTENT) != 0);
     }
 
     /**
@@ -132,7 +140,7 @@ private static boolean hasNormalVersion(int flags) {
 
     @Override
     public int compareTo(ModuleReference rhs) {
-        return NON_EMPTY_FIRST.compare(this, rhs);
+        return PREVIEW_FIRST.compare(this, rhs);
     }
 
     @Override
@@ -155,40 +163,62 @@ public int hashCode() {
     }
 
     /**
-     * Reads the content buffer of a package subdirectory to construct a list
-     * of module references.
+     * Reads the content buffer of a package subdirectory to return a sequence
+     * of module name offsets in the jimage.
      *
      * @param buffer the content buffer of an {@link ImageLocation} with type
      *     {@link ImageLocation.LocationType#PACKAGES_DIR PACKAGES_DIR}.
-     * @param previewMode if {@code true} include all reference; otherwise skip
-     *     preview-only ones.
-     * @param nameDecoder decoder for module names.
-     * @return the list of module references for the source package subdirectory.
+     * @param includeNormal whether to include name offsets for modules present
+     *     in normal (non-preview) mode.
+     * @param includePreview whether to include name offsets for modules present
+     *     in preview mode.
+     * @return an iterator of module name offsets.
      */
-    public static List<ModuleReference> read(
-            IntBuffer buffer, boolean previewMode, Function<Integer, String> nameDecoder) {
+    public static Iterator<Integer> readNameOffsets(
+            IntBuffer buffer, boolean includeNormal, boolean includePreview) {
         int bufferSize = buffer.capacity();
         if (bufferSize == 0 || (bufferSize & 0x1) != 0) {
             throw new IllegalArgumentException("Invalid buffer size");
         }
-        // In non-preview mode we might skip a very small number of preview-only
-        // entries, but it's not worth "right-sizing" the array for that.
-        List<ModuleReference> refs = new ArrayList<>(bufferSize / 2);
-        for (int i = 0; i < bufferSize; i += 2) {
-            int packageFlags = buffer.get(i);
-            if ((packageFlags & (FLAGS_HAS_NORMAL_VERSION | FLAGS_HAS_PREVIEW_VERSION)) == 0) {
-                throw new IllegalArgumentException("Invalid package flags");
+        int testFlags = (includeNormal ? FLAGS_HAS_NORMAL_VERSION : 0)
+                + (includePreview ? FLAGS_HAS_PREVIEW_VERSION : 0);
+        if (testFlags == 0) {
+            throw new IllegalArgumentException("Invalid flags");
+        }
+
+        return new Iterator<Integer>() {
+            private int idx = nextIdx(0);
+
+            int nextIdx(int idx) {
+                for (; idx < bufferSize; idx += 2) {
+                    // If any of the test flags are set, include this entry.
+                    if ((buffer.get(idx) & testFlags) != 0) {
+                        return idx;
+                    } else if (!includeNormal) {
+                        // Preview entries are first in the offset buffer, so we
+                        // can exit early (by returning the end index) if we are
+                        // only iterating preview entries, and have run out.
+                        break;
+                    }
+                }
+                return bufferSize;
+            }
+
+            @Override
+            public boolean hasNext() {
+                return idx < bufferSize;
             }
-            if (previewMode || hasNormalVersion(packageFlags)) {
-                ModuleReference modRef = new ModuleReference(nameDecoder.apply(buffer.get(i + 1)), packageFlags);
-                // Validate the unique non-empty reference (if it exists) is first.
-                if (!refs.isEmpty() && !modRef.isEmpty()) {
-                    throw new IllegalArgumentException("Only first reference can have content: " + modRef);
+
+            @Override
+            public Integer next() {
+                if (idx < bufferSize) {
+                    int nameOffset = buffer.get(idx + 1);
+                    idx = nextIdx(idx + 2);
+                    return nameOffset;
                 }
-                refs.add(modRef);
+                throw new NoSuchElementException();
             }
-        }
-        return refs;
+        };
     }
 
     /**
@@ -206,13 +236,16 @@ public static void write(
         if (buffer.capacity() != (2 * refs.size())) {
             throw new IllegalArgumentException("Incorrect output buffer capacity");
         }
-        for (int i = 0; i < refs.size(); i++) {
-            ModuleReference modRef = refs.get(i);
-            if (i > 0 && !modRef.isEmpty()) {
-                throw new IllegalArgumentException("Only first reference can be non-empty: " + modRef);
+        int withContent = 0;
+        for (ModuleReference modRef : refs) {
+            if (modRef.hasContent()) {
+                withContent++;
             }
             buffer.put(modRef.flags);
             buffer.put(nameEncoder.apply(modRef.name));
         }
+        if (withContent > 1) {
+            throw new IllegalArgumentException("At most one reference can have content: " + refs);
+        }
     }
 }

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageResourcesTree.java

@@ -174,12 +174,11 @@ List<ModuleReference> getModuleReferences() {
         }
 
         private int validate() {
-            // If there's a module for which this package has content, it should be first and unique.
             if (moduleReferences.isEmpty()) {
                 throw new IllegalStateException("Package must be associated with modules: " + getName());
             }
-            if (moduleReferences.stream().skip(1).anyMatch(ref -> !ref.isEmpty())) {
-                throw new IllegalStateException("Multiple modules contain non-empty package: " + getName() + "\n\t" + moduleReferences.stream().map(Objects::toString).collect(joining("\n\t")));
+            if (moduleReferences.stream().filter(ModuleReference::hasContent).count() > 1) {
+                throw new IllegalStateException("Multiple modules contain non-empty package: " + getName());
             }
             // Calculate the package's ImageLocation flags.
             boolean hasPreviewVersion =