[lworld] RFR: 8271959: [lworld] Convert Optional and VBS classes to value class [v2]

David Holmes dholmes at openjdk.org
Wed Feb 22 21:40:48 UTC 2023


On Wed, 22 Feb 2023 15:47:16 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Make copies of classes to be made value classes in src/java.base/valueclasses/classes 
>> Modify CompileJavaModules.gmk to trigger builds of values classes and construct a .jar for each module.
>> Add the jar files to $JAVA_HOME/lib/valueclasses/<module\>-valueclasses.jar 
>> 
>> Modify hotspot arguments.cpp to scan for patch jar files when --enable-preview and -XX:+EnableValhalla.
>> For each jar, the equivalent of --patch-module <module>=<path-to-jar> is added and the system properties `jdk.module.patch.<n>` include jar file paths.
>
> Roger Riggs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Modify hotspot test for duplicate modules to check error message from the launcher
>   instead of ExceptionInInitilizer.

Mostly seems reasonable - though I'm not that familiar with the existing module patching logic. One issue with tracking `patch_mod_javabase`.

src/hotspot/share/runtime/arguments.cpp line 2221:

> 2219:       *(module_name + module_len) = '\0';
> 2220:       // The path piece begins one past the module_equal sign
> 2221:       add_patch_mod_prefix(module_name, module_equal + 1, false);

`patch_mod_javabase` seems unused here now. Not obvious that this flag is being tracked properly with the new changes.

src/hotspot/share/runtime/arguments.cpp line 2246:

> 2244: 
> 2245:     jio_snprintf(valueclasses_dir, JVM_MAXPATHLEN, "%s%slib%s" VALUECLASS_STR "%s",
> 2246:         Arguments::get_java_home(), fileSep, fileSep, fileSep);

Nit: please indent continuation line so 'A' and 'v' line up.

src/hotspot/share/runtime/arguments.cpp line 2263:

> 2261: 
> 2262:         jio_snprintf(path, JVM_MAXPATHLEN, "%s%s", valueclasses_dir, &entry->d_name);
> 2263:         add_patch_mod_prefix(module_name, path, true);

Please document the `true` e.g. `true /* append */` (similar for the false case elsewhere)

-------------

PR: https://git.openjdk.org/valhalla/pull/816



More information about the valhalla-dev mailing list