Re: How to name modules, automatic and otherwise

[Stephan Herrmann]

On 02/17/2017 12:19 AM, Stephen Colebourne wrote:

The simplest and most consistent option is reverse DNS everywhere.
Everyone understand it and few will object!

An alternative option would be that open source can use short names,
but companies "must" use reverse DNS. But this is far from ideal given
how projects move from private to public, or how companies merge.

Another alternative is some form of group, that may or may not map
onto maven's group, where most of the time it does not have to be
specified:

module mainlib from com.mycompany {
  requires base;  // implicit, favours group 'com.mycompany' if there is a clash
  requires willow;  // uses 'com.mycompany' because there is a clash
  requires willow from org.joda;  // explicitly specified, but only
needed to resolve a clash
}


From here, wouldn't it be trivial to change Mark's counter example:

module com.bar:foo.data {
exports com.bar.foo.data;
requires org.hibernate:hibernate.core;
requires org.hibernate:hibernate.jcache;
requires org.hibernate:hibernate.validator;
}

into a positive example:

import org.hibernate:*;
module com.bar:foo.data {
exports com.bar.foo.data;
requires hibernate.core;
requires hibernate.jcache;
requires hibernate.validator;
}

best,
Stephan

Re: How to name modules, automatic and otherwise

[Stephen Colebourne]

On 16 February 2017 at 16:48,   wrote:
>  This can be done very simply, with a single new JAR-file manifest 
> `Module-Name` attribute

I welcome this.

> The reversed domain-name approach was sensible in the early days of Java,
> before we had development tools sophisticated enough to help us deal with
> the occasional conflict.  We have such tools now, so going forward the
> superior readability of short module and package names that start with
> project or product names is preferable to the onerous verbosity of those
> that start with reversed domain names.

What tools?

With short identifiers clashes are inevitable. Because the module name
is baked inside the module in binary format, the only way to resolve
the clash is to rewrite the module. The Java platform has not demanded
anything like this before, and I can't see how it meets the reliable
configuration requirement. Rewriting modules as part of the build
system is a red line for me. I need to be able to see that the module
on the module path is the same bits as that from the source of jars.

The standard case to consider is as follows:

- In 2017, a company creates an internal foundation library called
"willow" and it becomes very popular within the company and is used
100s of times
- In 2018, an unrelated open source project starts up with the name
"willow" and becomes very popular. Both now publish modules with the
name "willow" (one privately, one publicly).
- In 2019, the company wants to use the open source "willow" library
(directly or indirectly), but can't due to name clash
- In 2020, the company wants to open source their "willow" library,
but can't due to name clash

The plan outlined, favouring short IDs, provides no solution to this
problem that I can see. There simply isn't the breadth of identifier
to avoid clashes like this (you can't possibly predict the future
where you might need to coexist with an open source module that
doesn't even exist yet). Proposal (A) only tackles automatic modules,
and not the bigger problem where names are baked into the module
itself.

The simplest and most consistent option is reverse DNS everywhere.
Everyone understand it and few will object!

An alternative option would be that open source can use short names,
but companies "must" use reverse DNS. But this is far from ideal given
how projects move from private to public, or how companies merge.

Another alternative is some form of group, that may or may not map
onto maven's group, where most of the time it does not have to be
specified:

module mainlib from com.mycompany {
  requires base;  // implicit, favours group 'com.mycompany' if there is a clash
  requires willow;  // uses 'com.mycompany' because there is a clash
  requires willow from org.joda;  // explicitly specified, but only
needed to resolve a clash
}

With this approach, the clash can be resolved, but only needs to be by
the first module in the graph to pull both in. Any transitive use of
the two willow modules would be fine.

In summary, I recognise the desire for short, pretty identifiers.
However, I remain of the opinion that they are highly dangerous for
the wider ecosystem without some additional ability to qualify them.
The are many more private jars than public jars, and the clashes seen
today on Maven Central are just the tip of the iceberg of this
problem.

Stephen

Re: Performance of JrtFileSystemProvider.exists

[Jason Zaugg]

Thanks, Alan. Good to hear that this is on the radar.

In the meantime, I think I can solve our performance problem by using the
/packages namespace to route lookups to the relevant module(s) to avoid so
many negative lookups in the first place.

Regards,

Jason



On Thu, 16 Feb 2017 at 18:44 Alan Bateman  wrote:

> On 16/02/2017 08:15, Jason Zaugg wrote:
>
> > Recently I modified the Scala compiler to read class files
> > jrt:/ filesystem, rather than relying on rt.jar.
> >
> > I noticed a slowdown when running on JDK 9 ea and exercising this code.
> > Profiles suggest that the bottleneck is in calls to Files.exists.
> > JrtFileSystemProvider inherits an implementation which needs to
> construct,
> > throw and catch NoSuchFileException to answer in the negative.
> >
> > It is possible that I'm calling this more often than needed, and could
> > implement a caching layer. But it seems worthwhile for jrtfs to
> implement a
> > fast path for exists, as is done in UnixFileSystemProvider.
> >
> > Possibly related bug "Startup regression with Jython":
> > https://bugs.openjdk.java.net/browse/JDK-8166236
> >
> As it happens, we a similar issue with the zip provider. It's something
> that I was looking at with Sherman recently and we have a proposal to
> push the exists down to the provider interface so that it can be
> overridden.
>
> So a different issues to JDK-8166236 - that one is that Jython seems to
> create a cache on first usage of all classes that it finds in rt.jar. I
> assume that if/when Jython adds support for JDK 9 and jrtfs that they
> will be able to integrate it with their caching.
>
> -Alan
>

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

[Mandy Chung]

One more thing: the warning message should be in plugin.properties
to be localized.

Mandy

> On Feb 16, 2017, at 12:04 PM, Mandy Chung  wrote:
> 
> 
>> On Feb 16, 2017, at 5:20 AM, Claes Redestad  
>> wrote:
>> 
>>> 
>>> In addition, if the main argument is specified but the version does not 
>>> match, it will ignore the given argument.  Should it be an error instead?  
>>> We are the one who will generate a trace file and specify it in the jlink 
>>> plugin option.  It’s okay to ignore the default trace output if no plugin 
>>> option is specified and I think no warning should be printed in this case.  
>>> It’s just like this plugin is disabled.  You may want to add a suboption to 
>>> turn on verbose that will trace what is generated and what is ignored.
>> 
>> I think a warning is reasonable in all cases: Using a different version of 
>> jlink than the java.base you're linking will lose some optimizations and the 
>> user would be none the wiser as to why, verbosity helps avoid surprises.
> 
> The plugin is enabled by default.  With this change, I consider
> this plugin is "auto-enabled" when it’s creating the image of
> the version that this plugin supports (i.e. matching major.minor
> version).
> 
> So if the —generate-jli-classes option is not specified, it might
> be confusing when I get this warning.  I would prefer in this case
> no warning should be emitted and the plugin is not enabled.
> 
> If the option is specified on the command-line, it should emit
> the warning.
> 
>> 
>> http://cr.openjdk.java.net/~redestad/8175026/jdk.02/
> 
> 322 if (!initialize(in)) {
> 
> Maybe refactor line 175-190 in a new method and something like this:
>   if (!checkVersion(getLinkedVersion(in)))
>   :
>   }
> 
> Then follow with initialize(in) here.  That’d make it explicit.
> One thing to handle is when exception is thrown when reading
> the trace file (default or mainArgument). Maybe that part can
> be done early in configure method and store the lines for later
> consumption.
> 
> line 235-238: you may use orThrow in this case.
> 
> Mandy

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

[Mandy Chung]

> On Feb 16, 2017, at 5:20 AM, Claes Redestad  wrote:
> 
>> 
>> In addition, if the main argument is specified but the version does not 
>> match, it will ignore the given argument.  Should it be an error instead?  
>> We are the one who will generate a trace file and specify it in the jlink 
>> plugin option.  It’s okay to ignore the default trace output if no plugin 
>> option is specified and I think no warning should be printed in this case.  
>> It’s just like this plugin is disabled.  You may want to add a suboption to 
>> turn on verbose that will trace what is generated and what is ignored.
> 
> I think a warning is reasonable in all cases: Using a different version of 
> jlink than the java.base you're linking will lose some optimizations and the 
> user would be none the wiser as to why, verbosity helps avoid surprises.

The plugin is enabled by default.  With this change, I consider
this plugin is "auto-enabled" when it’s creating the image of
the version that this plugin supports (i.e. matching major.minor
version).

So if the —generate-jli-classes option is not specified, it might
be confusing when I get this warning.  I would prefer in this case
no warning should be emitted and the plugin is not enabled.

If the option is specified on the command-line, it should emit
the warning.

> 
> http://cr.openjdk.java.net/~redestad/8175026/jdk.02/

 322 if (!initialize(in)) {

Maybe refactor line 175-190 in a new method and something like this:
   if (!checkVersion(getLinkedVersion(in)))
   :
   }
   
Then follow with initialize(in) here.  That’d make it explicit.
One thing to handle is when exception is thrown when reading
the trace file (default or mainArgument). Maybe that part can
be done early in configure method and store the lines for later
consumption.

line 235-238: you may use orThrow in this case.

Mandy

Re: How to name modules, automatic and otherwise

[Martin Balin]

Hello,
This looks reasonable. Is it going to be implemented in JDK9 prior GA date?
Martin

On 16.2.2017 17:48, mark.reinh...@oracle.com wrote:

Thanks to everyone for all the feedback on this topic.  I've posted my
conclusions here:

   
http://mail.openjdk.java.net/pipermail/jpms-spec-experts/2017-February/000582.html

- Mark

hg: jigsaw/jake: 2 new changesets

[alan . bateman]

Changeset: 72761323ba0a
Author:lana
Date:  2017-02-16 17:12 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/rev/72761323ba0a

Added tag jdk-9+157 for changeset 4eb77fb98952

! .hgtags

Changeset: d3c64c12b7c0
Author:alanb
Date:  2017-02-16 18:00 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/rev/d3c64c12b7c0

Merge

! .hgtags

hg: jigsaw/jake/jdk: 4 new changesets

[alan . bateman]

Changeset: 96748d4b1204
Author:jlahoda
Date:  2017-02-13 09:41 +0100
URL:   http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/96748d4b1204

8173777: Merge javac -Xmodule into javac--patch-module
Summary: Converting uses of -Xmodule: to --patch-module.
Reviewed-by: alanb, mchung

- test/javax/xml/jaxp/common/8035437/Document.java
- test/javax/xml/jaxp/common/8035437/DocumentImpl.java
- test/javax/xml/jaxp/common/8035437/Node.java
+ test/javax/xml/jaxp/common/8035437/patch-src1/org/w3c/dom/Document.java
+ test/javax/xml/jaxp/common/8035437/patch-src1/org/w3c/dom/Node.java
+ 
test/javax/xml/jaxp/common/8035437/patch-src2/com/sun/org/apache/xerces/internal/dom/DocumentImpl.java
! test/javax/xml/jaxp/common/8035437/run.sh
- test/sun/text/IntHashtable/Bug4170614Test.java
! test/sun/text/IntHashtable/Bug4170614Test.sh
+ test/sun/text/IntHashtable/patch-src/java/text/Bug4170614Test.java
! test/tools/launcher/ToolsOpts.java
! test/tools/launcher/modules/patch/basic/PatchTest.java
! test/tools/launcher/modules/patch/basic/PatchTestWarningError.java
! test/tools/launcher/modules/patch/systemmodules/PatchSystemModules.java

Changeset: fdfa7b2fe9a7
Author:mullan
Date:  2017-02-13 11:35 -0500
URL:   http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/fdfa7b2fe9a7

8174837: Add "since=9" to deprecated ContentSigner and ContentSignerParameters 
classes
Reviewed-by: vinnie

! src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSigner.java
! src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSignerParameters.java

Changeset: bdf3d3a46863
Author:lana
Date:  2017-02-16 17:13 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/bdf3d3a46863

Added tag jdk-9+157 for changeset fdfa7b2fe9a7

! .hgtags

Changeset: d67bb2021453
Author:alanb
Date:  2017-02-16 18:01 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/d67bb2021453

Merge

! .hgtags
- test/javax/xml/jaxp/common/8035437/Document.java
- test/javax/xml/jaxp/common/8035437/DocumentImpl.java
- test/javax/xml/jaxp/common/8035437/Node.java
! test/javax/xml/jaxp/common/8035437/run.sh
- test/sun/text/IntHashtable/Bug4170614Test.java
! test/tools/launcher/ToolsOpts.java
! test/tools/launcher/modules/patch/basic/PatchTest.java
! test/tools/launcher/modules/patch/systemmodules/PatchSystemModules.java

hg: jigsaw/jake/hotspot: 2 new changesets

[alan . bateman]

Changeset: 4e78f3093522
Author:lana
Date:  2017-02-16 17:12 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/hotspot/rev/4e78f3093522

Added tag jdk-9+157 for changeset b2d0a906afd7

! .hgtags

Changeset: f06b24e86ca5
Author:alanb
Date:  2017-02-16 18:10 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/hotspot/rev/f06b24e86ca5

Merge

! .hgtags

hg: jigsaw/jake/nashorn: 2 new changesets

[alan . bateman]

Changeset: 13ae2480a4c3
Author:lana
Date:  2017-02-16 17:13 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/nashorn/rev/13ae2480a4c3

Added tag jdk-9+157 for changeset f6070efba6af

! .hgtags

Changeset: a71b174558de
Author:alanb
Date:  2017-02-16 18:02 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/nashorn/rev/a71b174558de

Merge

! .hgtags

hg: jigsaw/jake/jaxws: 2 new changesets

[alan . bateman]

Changeset: 2dbdf7460052
Author:lana
Date:  2017-02-16 17:12 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jaxws/rev/2dbdf7460052

Added tag jdk-9+157 for changeset b7e70e1e0154

! .hgtags

Changeset: 2caee4d0ea4f
Author:alanb
Date:  2017-02-16 18:03 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jaxws/rev/2caee4d0ea4f

Merge

! .hgtags

hg: jigsaw/jake/langtools: 4 new changesets

[alan . bateman]

Changeset: 8be741555fa6
Author:jlahoda
Date:  2017-02-13 09:37 +0100
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/8be741555fa6

8173777: Merge javac -Xmodule into javac--patch-module
Summary: Merging -Xmodule: functionality into --patch-module.
Reviewed-by: jjg, mchung, rfield

! src/java.compiler/share/classes/javax/tools/StandardLocation.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/api/ClientCodeWrapper.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/code/ClassFinder.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/code/ModuleFinder.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Modules.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/main/Arguments.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
! src/jdk.compiler/share/classes/com/sun/tools/javac/main/Option.java
! 
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
! src/jdk.javadoc/share/classes/com/sun/tools/javadoc/main/ToolOption.java
! 
src/jdk.javadoc/share/classes/com/sun/tools/javadoc/resources/javadoc.properties
! src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ToolOption.java
! 
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/resources/javadoc.properties
! src/jdk.jshell/share/classes/jdk/jshell/MemoryFileManager.java
! test/tools/javac/6627362/T6627362.java
- test/tools/javac/6627362/x/Object.java
+ test/tools/javac/6627362/x/java/lang/Object.java
! test/tools/javac/diags/Example.java
! test/tools/javac/diags/examples.not-yet.txt
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleClassoutput/ModuleInfoWithPatchedModuleClassoutput.java
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleClassoutput/additional/module-info.java
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleClassoutput/patchmodule/java.compiler/javax/lang/model/element/Extra.java
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleSourcepath/ModuleInfoWithPatchedModule.java
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleSourcepath/patchmodule/java.compiler/javax/lang/model/element/Extra.java
+ 
test/tools/javac/diags/examples/ModuleInfoWithPatchedModuleSourcepath/patchmodule/java.compiler/module-info.java
- test/tools/javac/diags/examples/ModuleInfoWithXModuleSourcePath/Extra.java
- 
test/tools/javac/diags/examples/ModuleInfoWithXModuleSourcePath/module-info.java
- 
test/tools/javac/diags/examples/ModuleInfoWithXmoduleClasspath/ModuleInfoWithXmoduleClasspath.java
- 
test/tools/javac/diags/examples/ModuleInfoWithXmoduleClasspath/additional/module-info.java
- test/tools/javac/diags/examples/NoSuperclass.java
+ test/tools/javac/diags/examples/NoSuperclass/NoSuperclass.java
+ 
test/tools/javac/diags/examples/NoSuperclass/patchmodule/java.base/java/lang/Object.java
+ 
test/tools/javac/diags/examples/TooManyPatchedModules/TooManyPatchedModules.java
+ 
test/tools/javac/diags/examples/TooManyPatchedModules/patchmodule/java.compiler/p/C.java
+ 
test/tools/javac/diags/examples/TooManyPatchedModules/patchmodule/jdk.compiler/p/C.java
- 
test/tools/javac/diags/examples/XModuleWithModulePath/XModuleWithModulePath.java
- test/tools/javac/meth/BadPolySig.java
+ test/tools/javac/meth/BadPolySig/BadPolySig.java
+ test/tools/javac/meth/BadPolySig/java.base/java/lang/invoke/MethodHandle.java
! test/tools/javac/modules/AddLimitMods.java
! test/tools/javac/modules/AddReadsTest.java
+ test/tools/javac/modules/CompileModulePatchTest.java
! test/tools/javac/modules/InheritRuntimeEnvironmentTest.java
! test/tools/javac/modules/PatchModulesTest.java
- test/tools/javac/modules/XModuleTest.java
! test/tools/javac/synthesize/Main.java
! test/tools/jdeps/jdkinternals/RemovedJDKInternals.java

Changeset: 162b521af7bb
Author:jlahoda
Date:  2017-02-13 11:57 +0100
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/162b521af7bb

8174245: Javadoc is not working for some methods
Summary: Parsing source file as if they were part of their corresponding 
modules.
Reviewed-by: rfield

! 
src/jdk.compiler/share/classes/jdk/internal/shellsupport/doc/JavadocHelper.java
! src/jdk.jshell/share/classes/jdk/internal/jshell/tool/ConsoleIOContext.java
! test/jdk/jshell/JavadocTest.java

Changeset: f9168e271f7d
Author:lana
Date:  2017-02-16 17:13 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/f9168e271f7d

Added tag jdk-9+157 for changeset 162b521af7bb

! .hgtags

Changeset: 33de8aa75ab9
Author:alanb
Date:  2017-02-16 18:01 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/33de8aa75ab9

Merge

! .hgtags
! src/java.compiler/share/classes/javax/tools/StandardLocation.java
! 

hg: jigsaw/jake/jaxp: 2 new changesets

[alan . bateman]

Changeset: 80143dad68ba
Author:lana
Date:  2017-02-16 17:12 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jaxp/rev/80143dad68ba

Added tag jdk-9+157 for changeset 412df235a8a2

! .hgtags

Changeset: 78a99c5871b1
Author:alanb
Date:  2017-02-16 18:02 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/jaxp/rev/78a99c5871b1

Merge

! .hgtags

hg: jigsaw/jake/corba: 2 new changesets

[alan . bateman]

Changeset: de6bdf38935f
Author:lana
Date:  2017-02-16 17:12 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/corba/rev/de6bdf38935f

Added tag jdk-9+157 for changeset 9383da04b385

! .hgtags

Changeset: a3f338a105fa
Author:alanb
Date:  2017-02-16 18:02 +
URL:   http://hg.openjdk.java.net/jigsaw/jake/corba/rev/a3f338a105fa

Merge

! .hgtags

Re: How to name modules, automatic and otherwise

[Brian Fox]

Thanks for the follow up Mark.

I obviously don't completely agree with all of the assumptions and that's
ok.

I do however applaud the inclusion of the Module-Name into the algorithm.
This gives enough of a hook that tools like Maven can hopefully send the
ecosystem down a smooth transition path, avoiding or reducing many of the
concerns we raised.

--Brian

On Thu, Feb 16, 2017 at 11:48 AM,  wrote:

> Thanks to everyone for all the feedback on this topic.  I've posted my
> conclusions here:
>
>   http://mail.openjdk.java.net/pipermail/jpms-spec-experts/
> 2017-February/000582.html
>
> - Mark
>

How to name modules, automatic and otherwise

[mark . reinhold]

Thanks to everyone for all the feedback on this topic.  I've posted my
conclusions here:

  
http://mail.openjdk.java.net/pipermail/jpms-spec-experts/2017-February/000582.html

- Mark

Re: RFR: 8175079: Lazy initialization of ImageReader breaks rmid

[Claes Redestad]

Done!

On 02/16/2017 05:27 PM, Alan Bateman wrote:

I think go with the first for now.

-Alan


On 16/02/2017 16:24, Claes Redestad wrote:


Hi,

please review this simple backout of a startup optimization that has 
proven to destabilize things like rmid. Patch inline..


Bug: https://bugs.openjdk.java.net/browse/JDK-8175079

diff -r 87f2a6fb4b9a src/java.base/share/classes/java/lang/System.java
--- a/src/java.base/share/classes/java/lang/System.javaWed Feb 15 
15:57:18 2017 +0100
+++ b/src/java.base/share/classes/java/lang/System.javaThu Feb 16 
17:18:49 2017 +0100

@@ -1945,9 +1945,6 @@
 // set security manager
 String cn = System.getProperty("java.security.manager");
 if (cn != null) {
-// ensure image reader for java.base is initialized 
before security manager

-Object.class.getResource("module-info.class");
-
 if (cn.isEmpty() || "default".equals(cn)) {
 System.setSecurityManager(new SecurityManager());
 } else {
diff -r 87f2a6fb4b9a 
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java
--- 
a/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java 
Wed Feb 15 15:57:18 2017 +0100
+++ 
b/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java 
Thu Feb 16 17:18:49 2017 +0100

@@ -115,12 +115,7 @@
 long t0 = System.nanoTime();

 // system modules (may be patched)
-ModuleFinder systemModules;
-if (SystemModules.MODULE_NAMES.length > 0) {
-systemModules = SystemModuleFinder.getInstance();
-} else {
-systemModules = ModuleFinder.ofSystem();
-}
+ModuleFinder systemModules = ModuleFinder.ofSystem();

 PerfCounters.systemModulesTime.addElapsedTimeFrom(t0);


An alternative patch is to move the force initialization of the image 
reader from initPhase3 to SecurityManager,
which ensures it's initialized before a security manager is 
installed. This preserves the startup optimization in

case a SM is not installed:

diff -r 87f2a6fb4b9a 
src/java.base/share/classes/java/lang/SecurityManager.java
--- a/src/java.base/share/classes/java/lang/SecurityManager.java Wed 
Feb 15 15:57:18 2017 +0100
+++ b/src/java.base/share/classes/java/lang/SecurityManager.java Thu 
Feb 16 17:23:57 2017 +0100

@@ -233,6 +233,11 @@
 public
 class SecurityManager {

+static {
+// ensure image reader for java.base is initialized before 
security

+// manager is installed
+Object.class.getResource("module-info.class");
+}
 /**
  * This field is true if there is a security check in
  * progress; false otherwise.


Thanks!

/Claes

Re: Running jaotc with an external Graal

[Mandy Chung]

> On Feb 16, 2017, at 1:33 AM, Doug Simon  wrote:
> 
> With the current bits in jdk9/hs and graal-core, the following bootstrapping 
> command works in terms of replacing Graal in the JDK:
> 
> java -server -XX:+UnlockExperimentalVMOptions 
> --module-path=/Users/dsimon/hs/truffle/mxbuild/modules/com.oracle.truffle.truffle_api.jar
>  
> --upgrade-module-path=/Users/dsimon/hs/graal-core/mxbuild/modules/jdk.vm.compiler.jar
>  --patch-module=jdk.vm.compiler=.jar -XX:+UseJVMCICompiler 
> -XX:+BootstrapJVMCI -version
> 
> However, the --patch-module + --upgrade-module-path trick[1] we’re using to 
> replace the version of Graal in the JDK apparently only works due to a bug 
> that will be fixed at some point. From Mandy Chung:
> 
> "-—patch-module is to patch a module to replace or add content of that module 
> and yes it disables the hash checking on the patched module. However, it’s 
> not intended to allow it to combine with —-upgrade-module-path as you did, to 
> change the module dependences.”
> 

It’s a bug that --patch-module + --upgrade-module-path work as described above. 
 I have a patch to fix —-patch-module that no longer has to disable the hash 
checking but allow patching the content of the mdoule.

I think we will need to look into the alternatives how Graal can be upgraded to 
a different version of module-info.class.

Mandy

> Given all the continuing flux around jigsaw, we cannot be sure that 
> developing and using GitHub Graal on JDK 9 is stable until jigsaw is stable.
> 
> -Doug
> 
> [1] 
> https://bugs.openjdk.java.net/browse/JDK-8171448?focusedCommentId=14046154=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14046154
> 
>> On 16 Feb 2017, at 10:17, Andrew Haley  wrote:
>> 
>> On 15/02/17 21:56, Christian Thalinger wrote:
>>> 
 On Feb 14, 2017, at 4:38 AM, Andrew Haley  wrote:
 
 On 14/02/17 14:37, Alan Bateman wrote:
> --patch-module can be used to patch any module in the boot layer. So if 
> you are looking to override or add classes then --patch-module should 
> work.
 
 Aha!  Thanks,
>>> 
>>> Does it?
>> 
>> Not quite.  The problem is that the service loader finds classes in
>> the Hotspot copy of Graal which don't exist in the external copy, and
>> chaos ensues.
>> 
>> I suppose the only way to make it work is to replace the copy of Graal
>> in HotSpot, but I don't think that's a trivial thing to do.
>> 
>> Andrew.
>> 
> 

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

[Claes Redestad]

Hi Mandy,

On 02/15/2017 08:09 PM, Mandy Chung wrote:

On Feb 15, 2017, at 9:12 AM, Claes Redestad  wrote:

Hi,

currently the file we generate at build time as input to
--generate-jli-classes is lost when linking custom images, which means
user generate images may perform worse in certain ways, mostly
generating more classes during startup.

Additionally, there's a strong assumption in --generate-jli-classes that
the VM running jlink is going to generate compatible classes with the
image being linked, which we can only really guarantee if the java.base
in the linked image is of the same version as the java.base in the VM
running jlink.  This patch tightens these checks to ensure we have
freedom to evolve and re-evaluate the implementation in future
releases.

JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/


This restriction sounds reasonable and we can enhance this in a future release. 
I think the plugin can record the configuration in its own format to be 
independent of the trace output format.

Instead of throwing ISA, you can have a test method to return a boolean to 
indicate if the default trace file should be read.

Instead of running java.base from the runtime, you can use Runtime.version() 
instead and I think comparing Version::major should be adequate.


Sure, I've changed to compare Runtime.Version.major and minor. While 
minor differences should be fine, the plugin depends on internals that 
are subject to change in minor
releases. To support experimentation and testing I've added a 
ignore-version suboption to override this.



This change disables this default setting entirely if the image being created 
is not the same version as this plugin (defaultSpecies and defaultInvokers, 
etc).  Is it intended?


Yes, this is intended, as we can't guarantee the code generated using 
one version of jlink will be compatible with the next; the format of the 
trace is also subject to change.




In addition, if the main argument is specified but the version does not match, 
it will ignore the given argument.  Should it be an error instead?  We are the 
one who will generate a trace file and specify it in the jlink plugin option.  
It’s okay to ignore the default trace output if no plugin option is specified 
and I think no warning should be printed in this case.  It’s just like this 
plugin is disabled.  You may want to add a suboption to turn on verbose that 
will trace what is generated and what is ignored.


I think a warning is reasonable in all cases: Using a different version 
of jlink than the java.base you're linking will lose some optimizations 
and the user would be none the wiser as to why, verbosity helps avoid 
surprises.


http://cr.openjdk.java.net/~redestad/8175026/jdk.02/

More verbosity control would be great, but that seems like a good fit 
for a JDK 10 RFE.


Thanks!

/Claes



Mandy

Re: Running jaotc with an external Graal

[Doug Simon]

With the current bits in jdk9/hs and graal-core, the following bootstrapping 
command works in terms of replacing Graal in the JDK:

java -server -XX:+UnlockExperimentalVMOptions 
--module-path=/Users/dsimon/hs/truffle/mxbuild/modules/com.oracle.truffle.truffle_api.jar
 
--upgrade-module-path=/Users/dsimon/hs/graal-core/mxbuild/modules/jdk.vm.compiler.jar
 --patch-module=jdk.vm.compiler=.jar -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI 
-version

However, the --patch-module + --upgrade-module-path trick[1] we’re using to 
replace the version of Graal in the JDK apparently only works due to a bug that 
will be fixed at some point. From Mandy Chung:

"-—patch-module is to patch a module to replace or add content of that module 
and yes it disables the hash checking on the patched module. However, it’s not 
intended to allow it to combine with —-upgrade-module-path as you did, to 
change the module dependences."

Given all the continuing flux around jigsaw, we cannot be sure that developing 
and using GitHub Graal on JDK 9 is stable until jigsaw is stable.

-Doug

[1] 
https://bugs.openjdk.java.net/browse/JDK-8171448?focusedCommentId=14046154=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14046154

> On 16 Feb 2017, at 10:17, Andrew Haley  wrote:
> 
> On 15/02/17 21:56, Christian Thalinger wrote:
>> 
>>> On Feb 14, 2017, at 4:38 AM, Andrew Haley  wrote:
>>> 
>>> On 14/02/17 14:37, Alan Bateman wrote:
 --patch-module can be used to patch any module in the boot layer. So if 
 you are looking to override or add classes then --patch-module should work.
>>> 
>>> Aha!  Thanks,
>> 
>> Does it?
> 
> Not quite.  The problem is that the service loader finds classes in
> the Hotspot copy of Graal which don't exist in the external copy, and
> chaos ensues.
> 
> I suppose the only way to make it work is to replace the copy of Graal
> in HotSpot, but I don't think that's a trivial thing to do.
> 
> Andrew.
> 

Re: Running jaotc with an external Graal

[Andrew Haley]

On 15/02/17 21:56, Christian Thalinger wrote:
> 
>> On Feb 14, 2017, at 4:38 AM, Andrew Haley  wrote:
>>
>> On 14/02/17 14:37, Alan Bateman wrote:
>>> --patch-module can be used to patch any module in the boot layer. So if 
>>> you are looking to override or add classes then --patch-module should work.
>>
>> Aha!  Thanks,
> 
> Does it?

Not quite.  The problem is that the service loader finds classes in
the Hotspot copy of Graal which don't exist in the external copy, and
chaos ensues.

I suppose the only way to make it work is to replace the copy of Graal
in HotSpot, but I don't think that's a trivial thing to do.

Andrew.

Re: RFR: 8175010: ImageReader is not thread-safe

[Claes Redestad]

Hi David,

On 2017-02-16 09:08, David Holmes wrote:

Hi Claes,

On 15/02/2017 11:22 PM, Claes Redestad wrote:

Hi,

a few intermittent but rare test failures[1] that has appeared
since the latest jake integration, and since one of the changes
in there was to make initialization of the system ImageReader
lazy there appears to be cases where ImageReaders are not
safely published:


I find it very hard to discern exactly how these classes are intended to
be used concurrently. Is there some lifecycle description of the
ImageReader and how it is intended to be used? Without any
synchronization I still see lots of not-thread-safe code. Simple example:

  75 public void close() throws IOException {
  76 if (closed) {
  77 throw new IOException("image file already closed");
  78 }
  79 reader.close(this);
  80 closed = true;
  81 }

two threads can still both call reader.close(this) concurrently. And if
the new closed volatile boolean solves something then wouldn't making
the reader variable volatile achieve the same JMM affects?


reader.close(this) calls will synchronize on 
SharedImageReader.OPEN_FILES, so concurrent calls on the same 
ImageReader should be OK.  We should add a comment to point

this out.

/Claes



David
-


- Ensure ImageReader::open is called only once per Path in
ImageReaderFactory by using CHM.computeIfAbsent
- Ensure ImageReader.reader is safely published to a
final field and signal close using a volatile boolean instead

webrev: http://cr.openjdk.java.net/~redestad/8175010/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8175010

Testing shows no issues (which admittedly doesn't mean we're
actually solving the root cause for JDK-8174817), and performance
numbers from adding a volatile read indicate that any overhead
is lost in the noise on ImageReader-heavy workloads.

Thanks!

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8174817

Re: RFR: 8175010: ImageReader is not thread-safe

[Claes Redestad]

On 2017-02-16 09:17, Peter Levart wrote:

I think your observations about potential issues in JRTFS is correct,
but there was nothing to suggest JRTFS code was involved in JDK-8174817
(as it's not code that's used by the BuiltinClassLoader).


The only remaining ImageReader.close() invocation is now in
jdk.internal.jrtfs.SystemImage anonymous inner class delegated from
SystemImage.close(), which is only called from JrtFileSystem.close() or
.finalize(). So you think JRTFS is not the source of the problem? What
if JRTFS is accessing the same image as JavaRuntimeURLConnection in the
same VM. Different instances of ImageReader would be used for them, but
they would share the same underlying SharedImageReader. Currently I
don't see a problem in SharedImageReader code, but I haven't studied it
carefully yet. Maybe there is something to be found there...



Nothing is impossible, there was simply no breadcrumbs to suggest this
was happening. One thing I think we should do to rule this out is to
make sure that the SharedImageReader representing the system image
simply can't be closed, which would really be an error.

/Claes

Re: Performance of JrtFileSystemProvider.exists

[Alan Bateman]

On 16/02/2017 08:15, Jason Zaugg wrote:


Recently I modified the Scala compiler to read class files
jrt:/ filesystem, rather than relying on rt.jar.

I noticed a slowdown when running on JDK 9 ea and exercising this code.
Profiles suggest that the bottleneck is in calls to Files.exists.
JrtFileSystemProvider inherits an implementation which needs to construct,
throw and catch NoSuchFileException to answer in the negative.

It is possible that I'm calling this more often than needed, and could
implement a caching layer. But it seems worthwhile for jrtfs to implement a
fast path for exists, as is done in UnixFileSystemProvider.

Possibly related bug "Startup regression with Jython":
https://bugs.openjdk.java.net/browse/JDK-8166236

As it happens, we a similar issue with the zip provider. It's something 
that I was looking at with Sherman recently and we have a proposal to 
push the exists down to the provider interface so that it can be overridden.


So a different issues to JDK-8166236 - that one is that Jython seems to 
create a cache on first usage of all classes that it finds in rt.jar. I 
assume that if/when Jython adds support for JDK 9 and jrtfs that they 
will be able to integrate it with their caching.


-Alan

Re: RFR: 8175010: ImageReader is not thread-safe

[Peter Levart]

Hi Claes,

On 02/15/2017 10:41 PM, Claes Redestad wrote:

Hi Peter,

are you suggesting that if I have class Foo { Bar b; }, then creating
and putting Foo b in a CHM before returning it to a consumer which is
then read from another thread is enough to force b to be safely
published even when the other thread does *not* get the object via a
call to the same CHM (which would go via the same volatile read and add
the necessary happens before relationship)?  I recalled having seen
examples to the effect that a non-volatile b isn't safely published in
this case.


No, I'm not suggesting that. I'm merely observing (and hoping that my 
IDE didn't miss places in code where this is not the case) that there is 
no unsafe publication going on here. Obtaining ImageReader from 
ImageReaderFactory.get() is safe as it either returns an instance 
constructed in the same thread or it returns an instance constructed in 
a different thread, but in that case, the instance is handed through 
CHM.set/get which enforces ordering. The only other place where I found 
ImageReader.open() call is in constructing the SystemImage 
implementation as an anonymous inner instance which holds an ImageReader 
in its synthetic final field which ensures safe publication of 
ImageReader even if SystemImage instance is published unsafely.




The (very shaky) hypothesis is thus that this could be what's happening
in any of the places which load and locally cache the system ImageReader
for anyone to use, e.g., SystemModuleFinder.SystemImage 


...here the ImageReader instance is obtained via 
ImageReaderFactory.getImageReader():


/**
 * Holder class for the ImageReader
 */
private static class SystemImage {
static final ImageReader READER;
static {
long t0 = System.nanoTime();
READER = ImageReaderFactory.getImageReader();
initTime.addElapsedTimeFrom(t0);
}

static ImageReader reader() {
return READER;
}
}

...which is just a shortcut for 
ImageReaderFactory.get(BOOT_MODULES_JIMAGE) and I believe 
ImageReaderFactory.get() publishes (and did publish even before the 
patch) ImageReader instances safely (old code):


  52 private static final Map readers = new 
ConcurrentHashMap<>();

  53
  54 /**
  55  * Returns an {@code ImageReader} to read from the given image 
file

  56  */
  57 public static ImageReader get(Path jimage) throws IOException {
  58 Objects.requireNonNull(jimage);
  59 ImageReader reader = readers.get(jimage);
  60 if (reader != null) {
  61 return reader; // <<-- HERE the instance was handed 
through CHM

  62 }
  63 reader = ImageReader.open(jimage);
  64 // potential race with other threads opening the same URL
  65 ImageReader r = readers.putIfAbsent(jimage, reader);
  66 if (r == null) {
  67 return reader; // <<-- HERE the instance was 
constructed in same thread

  68 } else {
  69 reader.close();
  70 return r; // <<-- HERE the instance was handed through CHM
  71 }
  72 }



or
JavaRuntimeURLConnection (which is implicitly called when there's a
security manager installed).


...here the instance is also obtained via 
ImageReaderFactory.getImageReader():


// ImageReader to access resources in jimage
private static final ImageReader reader;
static {
PrivilegedAction pa = 
ImageReaderFactory::getImageReader;

reader = AccessController.doPrivileged(pa);
}


In both above places the ImageReader instance is obtained in  
and this is another "layer" of synchronization between consumer and 
producer threads.



I might be (in fact likely am) wrong, but
we discussed this offline and came to the conclusion that there was no
harm in implementing these improvements regardless of whether they
actually resolve 8174817 or not.


No, there's no harm and I support the changes. It's just that they might 
not help in resolving the problem.




I think prior to this patch a concurrent ImageReader.close() could
happen if there was a race between 3 or more threads to resolve the
same Path from ImageReaderFactory.get (especially since there might be
a longish time window there since we might block to load a library
etc), so I don't think we lose anything from plugging that hole by
using computeIfAbsent here.


I don't believe this was the the source of concurrent close(). If you 
look at the code (above), you can see that reader.close() (line 69) is 
called only if the reader was not successfully published and it is 
called by the thread that constructed it. But computeIfAbsent is a good 
choice here to avoid optimistic constructions followed by close(s) when 
there is concurrency.




I think your observations about potential issues in JRTFS is correct,
but there was nothing to suggest JRTFS code was involved in JDK-8174817
(as it's 

Performance of JrtFileSystemProvider.exists

[Jason Zaugg]

Recently I modified the Scala compiler to read class files
jrt:/ filesystem, rather than relying on rt.jar.

I noticed a slowdown when running on JDK 9 ea and exercising this code.
Profiles suggest that the bottleneck is in calls to Files.exists.
JrtFileSystemProvider inherits an implementation which needs to construct,
throw and catch NoSuchFileException to answer in the negative.

It is possible that I'm calling this more often than needed, and could
implement a caching layer. But it seems worthwhile for jrtfs to implement a
fast path for exists, as is done in UnixFileSystemProvider.

Possibly related bug "Startup regression with Jython":
https://bugs.openjdk.java.net/browse/JDK-8166236

Regards,

Jason Zaugg

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

[Magnus Ihse Bursie]

On 2017-02-15 18:12, Claes Redestad wrote:

Hi,

currently the file we generate at build time as input to
--generate-jli-classes is lost when linking custom images, which means
user generate images may perform worse in certain ways, mostly
generating more classes during startup.

Additionally, there's a strong assumption in --generate-jli-classes that
the VM running jlink is going to generate compatible classes with the
image being linked, which we can only really guarantee if the java.base
in the linked image is of the same version as the java.base in the VM
running jlink.  This patch tightens these checks to ensure we have
freedom to evolve and re-evaluate the implementation in future
releases.

JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8175026

Thanks!


Build changes look good.

/Magnus