8178380: Module system implementation refresh (5/2017 update)

[Mandy Chung]

> On May 1, 2017, at 1:28 PM, Alan Bateman <alan.bateman at oracle.com> wrote:
> 
> The webrevs with the changes is here:
>    http://cr.openjdk.java.net/~alanb/8178380/1/

I have reviewed all repos.  Mostly looks good.  This is the first round of comments.  I will continue the review and plan to play a little more with the new launcher options.

src/share/vm/services/attachListener.cpp

 355 // Dynamic loading agents is not default by default

typo: s/not default/not enabled?

src/share/vm/services/diagnosticCommand.cpp
 771     loadAgentModule(CHECK);
 772     Handle loader = Handle(THREAD, SystemDictionary::java_system_loader());
 773     Klass* k = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_agent_Agent(), loader, Handle(), true, CHECK);
 774     instanceKlassHandle ik (THREAD, k);

It’s better to refactor this to loadAgentClass that returns Klass or handle?

src/java.base/share/classes/java/lang/ClassLoader.java

 134  *     platform class loader may delegate to application class loader.

“the” application class loader

 135  *     In other words, classes in modules defined to the application class
 136  *     loader may be visible to the platform class loader. </li>

It would help if this statement makes it clear that the classes in modules defined to the application class loader (only that are required by the upgradeable modules) are visible to the platform class loader.

It would also be good to mention that ClassLoader::getPackages will not 
return the packages defined to the application class loader since
application class loader is not its ancestor.

 377      * @apiNote If the parent is specified as {@code null} (for the
 378      * bootstrap class loader) then there is no guarantee that all platform
 379      * classes are visible.

What about: 

If the parent is specified as {@code null} (for the bootstrap class loader)
then there is no guarantee that all platform classes are visible.
Platform classes defined by the platform class loader and its ancestors
except bootstrap class loader are not visible to this class loader.

src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java

 174     /**
 175      * Register a module this this class loader. This has the effect of making
 176      * the types in the module visible.
 177      */
 178     public void loadModule(ModuleReference mref) {

Typo in line 175 “this this class loader”.

src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java

147         // special mode to boot with only java.base, ignores other options
 148         if (System.getProperty("jdk.module.minimumBoot") != null) {
 149             return createMinimalBootLayer();
 150         }

src/java.base/share/classes/jdk/internal/module/ModulePatcher.java
 123                     try (JarFile jf = new JarFile(file.toString())) {
- what is the bug being fixed by this line?

src/java.base/share/classes/jdk/internal/module/ModulePath.java
 536                     if (packages.contains(pn)) builder.mainClass(mainClass);
Nit: it would be consistent if this is broken into two lines.

src/java.base/share/classes/sun/launcher/LauncherHelper.java
 960         bootLayer.modules().stream()
 961             .map(m -> cf.findModule(m.getName()))
 962             .flatMap(Optional::stream)

This can be replaced with cf.modules().stream()

 988             .map(e -> Stream.concat(Stream.of(e.source()),
 989                     toStringStream(e.modifiers()))
Formatting nit: line 989 should be indented more to the right

1064      * image ot be less than modules than not in the run-time image.

typo: “ot”

Should these new tracing methods drop the printToStderr argument?
This argument exists for compatibility to print help message to stderr.
These new tracing options will print to stdout.  For validate modules,
you suggested to print the errors to stderr which I agree.

src/java.base/share/classes/sun/launcher/resources/launcher.properties
  60 \    --limit-modules <module name>[,<module name>...]\n\
  61 \                  limit the universe of observable modules\n\

-—limit-modules is intended for testing purpose.  I wonder if 
this should be moved to —-extra-help.

  72 \                  The --validate-modules option may be is useful for finding\n\

typo: “may be is useful”

src/jdk.jartool/share/classes/sun/tools/jar/Main.java
 619                         if (dflag) {
 620                             // "--describe-module/-d" does not require file argument(s),
 621                             // but does accept --release
 622                             usageError(getMsg("error.bad.dflag"));
 623                             return false;
 624                         }


Mandy