Updates and test fixing.

* Fix test for new ModuleReference behaviour.
* Make preview mode system property dependent in classloader.
* Update benchmark for preview mode and new ImageReader methods.

Author: david-beaumont

src/hotspot/share/runtime/arguments.cpp

@@ -2079,56 +2079,69 @@ int Arguments::process_patch_mod_option(const char* patch_mod_tail) {
   return JNI_OK;
 }
 
+// Temporary system property to disable preview patching and enable the new preview mode
+// feature for testing/development. Once the preview mode feature is finished, the value
+// will be always 'true' and this code, and all related dead-code can be removed.
+#define DISABLE_PREVIEW_PATCHING_DEFAULT false
+
+bool Arguments::disable_preview_patching() {
+  const char* prop = get_property("DISABLE_PREVIEW_PATCHING");
+  return (prop != nullptr)
+      ? strncmp(prop, "true", strlen("true")) == 0
+      : DISABLE_PREVIEW_PATCHING_DEFAULT;
+}
+
 // VALUECLASS_STR must match string used in the build
 #define VALUECLASS_STR "valueclasses"
 #define VALUECLASS_JAR "-" VALUECLASS_STR ".jar"
 
 // Finalize --patch-module args and --enable-preview related to value class module patches.
 // Create all numbered properties passing module patches.
 int Arguments::finalize_patch_module() {
-  // If --enable-preview and EnableValhalla is true, each module may have value classes that
-  // are to be patched into the module.
+  // If --enable-preview and EnableValhalla is true, modules may have preview mode resources.
   bool enable_valhalla_preview = enable_preview() && EnableValhalla;
+  // Whether to use module patching, or the new preview mode feature for preview resources.
+  bool disable_patching = disable_preview_patching();
 
   // This must be called, even with 'false', to enable resource lookup from JImage.
-  ClassLoader::init_jimage(enable_valhalla_preview);
+  ClassLoader::init_jimage(disable_patching && enable_valhalla_preview);
+
+  // For each <module>-valueclasses.jar in <JAVA_HOME>/lib/valueclasses/
+  // appends the equivalent of --patch-module <module>=<JAVA_HOME>/lib/valueclasses/<module>-valueclasses.jar
+  if (!disable_patching && enable_valhalla_preview) {
+    char * valueclasses_dir = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
+    const char * fileSep = os::file_separator();
+
+    jio_snprintf(valueclasses_dir, JVM_MAXPATHLEN, "%s%slib%s" VALUECLASS_STR "%s",
+                 Arguments::get_java_home(), fileSep, fileSep, fileSep);
+    DIR* dir = os::opendir(valueclasses_dir);
+    if (dir != nullptr) {
+      char * module_name = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
+      char * path = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
+
+      for (dirent * entry = os::readdir(dir); entry != nullptr; entry = os::readdir(dir)) {
+        // Test if file ends-with "-valueclasses.jar"
+        int len = (int)strlen(entry->d_name) - (sizeof(VALUECLASS_JAR) - 1);
+        if (len <= 0 || strcmp(&entry->d_name[len], VALUECLASS_JAR) != 0) {
+          continue;         // too short or not the expected suffix
+        }
 
-//  // For each <module>-valueclasses.jar in <JAVA_HOME>/lib/valueclasses/
-//  // appends the equivalent of --patch-module <module>=<JAVA_HOME>/lib/valueclasses/<module>-valueclasses.jar
-//  if (enable_valhalla_preview) {
-//    char * valueclasses_dir = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
-//    const char * fileSep = os::file_separator();
-//
-//    jio_snprintf(valueclasses_dir, JVM_MAXPATHLEN, "%s%slib%s" VALUECLASS_STR "%s",
-//                 Arguments::get_java_home(), fileSep, fileSep, fileSep);
-//    DIR* dir = os::opendir(valueclasses_dir);
-//    if (dir != nullptr) {
-//      char * module_name = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
-//      char * path = AllocateHeap(JVM_MAXPATHLEN, mtArguments);
-//
-//      for (dirent * entry = os::readdir(dir); entry != nullptr; entry = os::readdir(dir)) {
-//        // Test if file ends-with "-valueclasses.jar"
-//        int len = (int)strlen(entry->d_name) - (sizeof(VALUECLASS_JAR) - 1);
-//        if (len <= 0 || strcmp(&entry->d_name[len], VALUECLASS_JAR) != 0) {
-//          continue;         // too short or not the expected suffix
-//        }
-//
-//        strcpy(module_name, entry->d_name);
-//        module_name[len] = '\0';     // truncate to just module-name
-//
-//        jio_snprintf(path, JVM_MAXPATHLEN, "%s%s", valueclasses_dir, &entry->d_name);
-//        add_patch_mod_prefix(module_name, path, true /* append */, true /* cds OK*/);
-//        log_info(class)("--enable-preview appending value classes for module %s: %s", module_name, entry->d_name);
-//      }
-//      FreeHeap(module_name);
-//      FreeHeap(path);
-//      os::closedir(dir);
-//    }
-//    FreeHeap(valueclasses_dir);
-//  }
+        strcpy(module_name, entry->d_name);
+        module_name[len] = '\0';     // truncate to just module-name
+
+        jio_snprintf(path, JVM_MAXPATHLEN, "%s%s", valueclasses_dir, &entry->d_name);
+        add_patch_mod_prefix(module_name, path, true /* append */, true /* cds OK*/);
+        log_info(class)("--enable-preview appending value classes for module %s: %s", module_name, entry->d_name);
+      }
+      FreeHeap(module_name);
+      FreeHeap(path);
+      os::closedir(dir);
+    }
+    FreeHeap(valueclasses_dir);
+  }
 
   // Create numbered properties for each module that has been patched either
-  // by --patch-module or --enable-preview
+  // by --patch-module (or --enable-preview if disable_patching is false).
   // Format is "jdk.module.patch.<n>=<module_name>=<path>"
   if (_patch_mod_prefix != nullptr) {
     char * prop_value = AllocateHeap(JVM_MAXPATHLEN + JVM_MAXPATHLEN + 1, mtArguments);

src/hotspot/share/runtime/arguments.hpp

@@ -486,6 +486,8 @@ class Arguments : AllStatic {
   // Set up the underlying pieces of the boot class path
   static void add_patch_mod_prefix(const char *module_name, const char *path, bool allow_append, bool allow_cds);
   static int finalize_patch_module();
+  static bool disable_preview_patching();
+
   static void set_boot_class_path(const char *value, bool has_jimage) {
     // During start up, set by os::set_boot_path()
     assert(get_boot_class_path() == nullptr, "Boot class path previously set");

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

@@ -222,30 +222,44 @@ public Integer next() {
     }
 
     /**
-     * Writes a list of module references to a given buffer.
+     * Writes a list of module references to a given buffer. The given references
+     * list is checked carefully to ensure the written buffer will be valid.
      *
      * <p>Entries are written in order, taking two integer slots per entry as
      * {@code [<flags>, <encoded-name>]}.
      *
      * @param refs the references to write, correctly ordered.
      * @param buffer destination buffer.
      * @param nameEncoder encoder for module names.
+     * @throws IllegalArgumentException in the references are invalid in any way.
      */
     public static void write(
             List<ModuleReference> refs, IntBuffer buffer, Function<String, Integer> nameEncoder) {
-        if (buffer.capacity() != (2 * refs.size())) {
-            throw new IllegalArgumentException("Incorrect output buffer capacity");
+        if (refs.isEmpty()) {
+            throw new IllegalArgumentException("References list must be non-empty");
         }
-        int withContent = 0;
-        for (ModuleReference modRef : refs) {
-            if (modRef.hasContent()) {
-                withContent++;
+        int expectedCapacity = 2 * refs.size();
+        if (buffer.capacity() != expectedCapacity) {
+            throw new IllegalArgumentException(
+                    "Invalid buffer capacity: expected " + expectedCapacity + ", got " + buffer.capacity());
+        }
+        // This catches exact duplicates in the list.
+        refs.stream().reduce((lhs, rhs) -> {
+            if (lhs.compareTo(rhs) >= 0) {
+                throw new IllegalArgumentException("References must be strictly ordered: " + refs);
             }
-            buffer.put(modRef.flags);
-            buffer.put(nameEncoder.apply(modRef.name));
+            return rhs;
+        });
+        // Distinct references can have the same name (but we don't allow this).
+        if (refs.stream().map(ModuleReference::name).distinct().count() != refs.size()) {
+            throw new IllegalArgumentException("Reference names must be unique: " + refs);
         }
-        if (withContent > 1) {
+        if (refs.stream().filter(ModuleReference::hasContent).count() > 1) {
             throw new IllegalArgumentException("At most one reference can have content: " + refs);
         }
+        for (ModuleReference modRef : refs) {
+            buffer.put(modRef.flags);
+            buffer.put(nameEncoder.apply(modRef.name));
+        }
     }
 }