On Fri, 20 Oct 2023 14:34:52 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> simplify some code in modules.cpp
>
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 472:
>
>> 470:
>> 471: // If -Xshare:dump and mainModule are specified, check if the
>> mainModule
>> 472: // is in the runtime image and not in the upgrade module path.
>> If so,
>
> Small typo here, should be "on the upgrade module path" rather than "in".
Fixed
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 481:
>
>> 479: canArchive = true;
>> 480: }
>> 481: }
>
> An alternative to avoid the 3 levels of if expressions is to use:
>
> String scheme = systemModuleFinder.find(mainModule)
> .stream()
> .map(ModuleReference::location)
> .flatMap(Optional::stream)
> .findAny()
> .map(URI::getScheme)
> .orElse(null);
> if ("jrt".equalsIgnoreCase(scheme)) {
> canArchive = true;
> }
I tried the above but got the following build error:
Optimizing the exploded image
Error occurred during initialization of boot layer
java.lang.NullPointerException
ExplodedImageOptimize.gmk:39: recipe for target
'/scratch/cccheung/ws/git/curr_jdk/build/mywork-debug/jdk/_optimize_image_exec.marker'
failed
Let's keep the existing code for now because I may need to refactor the code
into a utility method for a upcoming RFE to support the --add-modules option.
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 491:
>
>> 489: cf,
>> 490: clf);
>> 491: if (!hasSplitPackages) {
>
> Did you mean to drop the hasIncubatorModules check? Incubator modules are not
> resolved by default.
Yes, because of the following code further up in the same method:
if (!haveModulePath && addModules.isEmpty() &&
limitModules.isEmpty()) {
systemModules = SystemModuleFinders.systemModules(mainModule);
if (systemModules != null && !isPatched) {
needResolution = (traceOutput != null);
canArchive = true;
}
}
if (systemModules == null) {
// all system modules are observable
systemModules = SystemModuleFinders.allSystemModules();
}
`SystemModuleFinders.systemModules(mainModule)` returns null if `mainModule` is
not null (I'm not sure why).
Then, the `SystemModuleFinders.allSystemModules()` returns the generated class
`SystemModules$all` with the following method:
public boolean hasIncubatorModules() {
return true;
}
Also, there's the following change:
`needResolution = (traceOutput != null) || CDS.isDumpingStaticArchive();`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1367457252
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1367457043
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1367455686
