Re: 8154956: Module system implementation refresh (4/2016)

[Chris Hegarty]

> On 29 Apr 2016, at 13:38, Alan Bateman  wrote:
> 
> The webrevs, all repos are here:
>  http://cr.openjdk.java.net/~alanb/8154956/1/

The updated changes look good to me.

-Chris.

Re: 8154956: Module system implementation refresh (4/2016)

[Peter Levart]

On 05/02/2016 08:24 AM, Alan Bateman wrote:


ModuleReader:
- the default method read(String name) should close the InputStream 
after reading from it, shouldn't it?
Yes it should, just hasn't been noticed because each of the 
implementations implement this method. I don't know if you mean to put 
the try on the same line but I'll make that a bit more readable before 
pushing the patch.


Good, it's more readable this way.

JmodModuleReader and JarModuleReader use this method, but this hasn't 
presented a problem since all those streams are closed when the JarFile 
is closed. But if there were any custom ModuleReaders (it's a public 
API) not overriding the default method, there would be a possible leak.


Regards, Peter

Re: 8154956: Module system implementation refresh (4/2016)

[Alan Bateman]

On 02/05/2016 06:52, Andrej Golovnin wrote:

:

I don't know how often the method #deriveModuleDescriptor(JarFile)
is executed. But compiling regular expressions over and over doesn't really
helps from the performance standpoint of view:

  400 Matcher matcher = Pattern.compile("-(\\d+(\\.|$))").matcher(mn);

  415 mn =  mn.replaceAll("[^A-Za-z0-9]", ".")  // replace
non-alphanumeric
  416 .replaceAll("(\\.)(\\1)+", ".")   // collapse
repeating dots
  417 .replaceAll("^\\.", "")   // drop leading dots
  418 .replaceAll("\\.$", "");  // drop trailing dots

Maybe the regular expressions in the above lines should be precompiled
in static final fields (Pattern objects are immutable and thread safe)
to improve
the performance of the #deriveModuleDescriptor(JarFile)-method.

This is the pattern matching to derive the module name for an automatic 
module. You are right that this is inefficient, we should only need to 
compile the pattern once. Probably not a static field though as it also 
has to be lazy. I think we have enough in this update so if you don't 
mind, I would prefer to leave this until another refresh.


-Alan

Re: 8154956: Module system implementation refresh (4/2016)

[Andrej Golovnin]

Hi Peter,

src/java.base/share/classes/java/lang/module/ModuleReader.java

 146 if (oin.isPresent()) try (InputStream in = oin.get()) {
 147 return Optional.of(ByteBuffer.wrap(in.readAllBytes()));
 148 } else {
 149 return Optional.empty();
 150 }

I highly doubt that the line 146 is easy to read. Can you change it into:

 146 if (oin.isPresent()) {
 147 try (InputStream in = oin.get()) {
 148 return Optional.of(ByteBuffer.wrap(in.readAllBytes()));
 149 }
 150 }
 151 return Optional.empty();

And maybe you can add following changes to your patch too.

http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.02/src/java.base/share/classes/java/lang/module/ModulePath.java.html

The following lines use the String-based version of the #lastIndexOf-method.

341 int index = cf.lastIndexOf("/") + 1;
391 int i = fn.lastIndexOf(File.separator);
568 int index = cn.lastIndexOf("/");

They should be changed to use the char-based version.

I don't know how often the method #deriveModuleDescriptor(JarFile)
is executed. But compiling regular expressions over and over doesn't really
helps from the performance standpoint of view:

 400 Matcher matcher = Pattern.compile("-(\\d+(\\.|$))").matcher(mn);

 415 mn =  mn.replaceAll("[^A-Za-z0-9]", ".")  // replace
non-alphanumeric
 416 .replaceAll("(\\.)(\\1)+", ".")   // collapse
repeating dots
 417 .replaceAll("^\\.", "")   // drop leading dots
 418 .replaceAll("\\.$", "");  // drop trailing dots

Maybe the regular expressions in the above lines should be precompiled
in static final fields (Pattern objects are immutable and thread safe)
to improve
the performance of the #deriveModuleDescriptor(JarFile)-method.

Best regards,
Andrej Golovnin

Re: 8154956: Module system implementation refresh (4/2016)

[Peter Levart]

Hi,

Last minute findings...

In ModulePath, jarPackages() and jmodPackages() helper method both use 
toPackageName(ZipEntry entry) method which checks for "classes/" prefix 
and strips it away. But "classes" is a valid package name or prefix (in 
case it is found in a .jar file). So it's better to only strip it off 
for jmodPackages()...


http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.02/

Regards, Peter

On 05/02/2016 12:27 AM, Peter Levart wrote:

Hi,

I have some mostly stylish changes (which you can use or not) 
regarding usage of Optional and Stream(s) in package java.lang.module:


http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/

...with some optimizations/cleanups/fixes:

ModuleFinder.compose:
- the composed ModuleFinder.findAll() is more efficient this way as it 
doesn't need to call back to find().


ModulePath:
- there's unneeded overhead to call distinct() for a Stream that is 
later collected into Collectors.toSet() as the collector already 
uniquifies the elements.
- lines 432...437 (old) - you don't want to treat entries in 
SERVICES_PREFIX directory that end with ".class" as classFiles - so I 
changes the:
.filter(e -> (e.endsWith(".class") || 
e.startsWith(SERVICES_PREFIX)))

  to:
.filter(s -> (s.endsWith(".class") ^ 
s.startsWith(SERVICES_PREFIX)))


ModuleReader:
- the default method read(String name) should close the InputStream 
after reading from it, shouldn't it?


ModuleReferences:
- no need for 'closed' flag in SafeCloseModuleReader to be volatile as 
it is always accessed under lock (either read lock when reading or 
write lock when writing).


Resolver:
- Implemented caching of ResolvedModule(s) as they are entered into 
resolved graph so that there are no duplicates.
- Simplified last stage of iterative algorithm to build resolved 
graph. There's no need to construct a map of changes and apply it 
afterwards to 'g1' as changes can be applied individually for each 
module 'm1' in the graph 'g1' as they are collected.



Regards, Peter

On 04/29/2016 02:38 PM, Alan Bateman wrote:


There have several changes in jake that we need to bring into 
jdk9/dev. The main changes are:


1. The policy described in JEP 261 for the root modules to resolve 
when compiling code in the unnamed module or where the main class is 
loaded from the class file. This is a disruptive change and we need 
to get through the transition.


Related is a new token `ALL-DEFAULT` that can be specified to 
`-addmods` to resolve the same roots when the initial module is a 
named module. This will eventually replace `ALL-SYSTEM` but we can't 
remove that just yet. The launchers for javac, javadoc, jlink and a 
few other tools that load arbitrary code in unnamed modules are now 
compiled with this option.


2. The transition to the new form of -Xpatch. This is mostly changes 
in hotspot but there are changes in other repos too to drop or 
replace the old form of -Xpatch (boot cycle builds for example).


3. Removal of the old form of -XaddExports and -XaddReads. This has 
build + test changes.


4. The second phase of integrity hashing. With this phase then the 
build records in java.base the hashes of the non-upgradeable modules. 
This prevents accidental linking of standard/JDK modules from 
different JDK builds. The jar and jmod tools have updated options to 
support this.


5. Peter Levart's patch to replace the internal WeakSet in jlr.Module 
with a WeakKeyPair.


6. Updates to jlink option handling. The reason this went into 
jdk9/dev is that it is disruptive to FX packager and it's hard to 
coordinate with FX when it's a separate forest. The packager class 
that Chris Bensen has added to jlink will allows us to iterate on 
jlink with reduced risk of breaking FX.


There are several small bug fixes and clean-ups in several areas, 
we'll have a lot more of these for the next update.


One other thing to mention is that we've bumped the required version 
of jtreg as jtreg relies on-Xpatch to add test cases into modules and 
so it needed to be updated too.


The webrevs, all repos are here:
http://cr.openjdk.java.net/~alanb/8154956/1/

There are a couple of files in the webrevs that we probably won't 
bring to JDK 9 in this update:


i. We have a patch to IDL compiler in the corba repo that needs a 
more extensive fix.
ii. The javadoc change in ModuleFinder as there are still details to 
decide on how modular JARs work as multi-release JARs.


One other point is that the webrevs are against jdk-9+116 for now. 
I've done a test merge + build with the current jdk9/dev forest and 
there are only a few conflicts to fix. I will re-merge + test with 
jdk9/dev once we have agreed the changes for this update.


Finally, just to say that we'll probably continue in jake for a while 
after we get through this update. There are several design issues on 
the JSR issues list that will likely require a few iterations and a 
bit of bake 

Re: 8154956: Module system implementation refresh (4/2016)

[Peter Levart]

Hi,

I have some mostly stylish changes (which you can use or not) regarding 
usage of Optional and Stream(s) in package java.lang.module:


http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/

...with some optimizations/cleanups/fixes:

ModuleFinder.compose:
- the composed ModuleFinder.findAll() is more efficient this way as it 
doesn't need to call back to find().


ModulePath:
- there's unneeded overhead to call distinct() for a Stream that is 
later collected into Collectors.toSet() as the collector already 
uniquifies the elements.
- lines 432...437 (old) - you don't want to treat entries in 
SERVICES_PREFIX directory that end with ".class" as classFiles - so I 
changes the:
.filter(e -> (e.endsWith(".class") || 
e.startsWith(SERVICES_PREFIX)))

  to:
.filter(s -> (s.endsWith(".class") ^ 
s.startsWith(SERVICES_PREFIX)))


ModuleReader:
- the default method read(String name) should close the InputStream 
after reading from it, shouldn't it?


ModuleReferences:
- no need for 'closed' flag in SafeCloseModuleReader to be volatile as 
it is always accessed under lock (either read lock when reading or write 
lock when writing).


Resolver:
- Implemented caching of ResolvedModule(s) as they are entered into 
resolved graph so that there are no duplicates.
- Simplified last stage of iterative algorithm to build resolved graph. 
There's no need to construct a map of changes and apply it afterwards to 
'g1' as changes can be applied individually for each module 'm1' in the 
graph 'g1' as they are collected.



Regards, Peter

On 04/29/2016 02:38 PM, Alan Bateman wrote:


There have several changes in jake that we need to bring into 
jdk9/dev. The main changes are:


1. The policy described in JEP 261 for the root modules to resolve 
when compiling code in the unnamed module or where the main class is 
loaded from the class file. This is a disruptive change and we need to 
get through the transition.


Related is a new token `ALL-DEFAULT` that can be specified to 
`-addmods` to resolve the same roots when the initial module is a 
named module. This will eventually replace `ALL-SYSTEM` but we can't 
remove that just yet. The launchers for javac, javadoc, jlink and a 
few other tools that load arbitrary code in unnamed modules are now 
compiled with this option.


2. The transition to the new form of -Xpatch. This is mostly changes 
in hotspot but there are changes in other repos too to drop or replace 
the old form of -Xpatch (boot cycle builds for example).


3. Removal of the old form of -XaddExports and -XaddReads. This has 
build + test changes.


4. The second phase of integrity hashing. With this phase then the 
build records in java.base the hashes of the non-upgradeable modules. 
This prevents accidental linking of standard/JDK modules from 
different JDK builds. The jar and jmod tools have updated options to 
support this.


5. Peter Levart's patch to replace the internal WeakSet in jlr.Module 
with a WeakKeyPair.


6. Updates to jlink option handling. The reason this went into 
jdk9/dev is that it is disruptive to FX packager and it's hard to 
coordinate with FX when it's a separate forest. The packager class 
that Chris Bensen has added to jlink will allows us to iterate on 
jlink with reduced risk of breaking FX.


There are several small bug fixes and clean-ups in several areas, 
we'll have a lot more of these for the next update.


One other thing to mention is that we've bumped the required version 
of jtreg as jtreg relies on-Xpatch to add test cases into modules and 
so it needed to be updated too.


The webrevs, all repos are here:
  http://cr.openjdk.java.net/~alanb/8154956/1/

There are a couple of files in the webrevs that we probably won't 
bring to JDK 9 in this update:


i. We have a patch to IDL compiler in the corba repo that needs a more 
extensive fix.
ii. The javadoc change in ModuleFinder as there are still details to 
decide on how modular JARs work as multi-release JARs.


One other point is that the webrevs are against jdk-9+116 for now. 
I've done a test merge + build with the current jdk9/dev forest and 
there are only a few conflicts to fix. I will re-merge + test with 
jdk9/dev once we have agreed the changes for this update.


Finally, just to say that we'll probably continue in jake for a while 
after we get through this update. There are several design issues on 
the JSR issues list that will likely require a few iterations and a 
bit of bake time before we bring them to JDK 9.


-Alan

Re: 8154956: Module system implementation refresh (4/2016)

[Andrej Golovnin]

>> I have a stupid question: Is it guaranteed that entry.getFileName().toString 
>> always
>> returns a value in lowercase?
>> 
> For now, the implementation only treat files on the module path ending in 
> ".jar" as candidate modular JAR or automatic modules. You will get an 
> unrecognized module error with `java -modulepath foo.JAR ..` for example. 
> You'll see the same in the tools. This has not changed in this refresh.

Ah, good to know! :-) Thanks for the info, Alan!

Best regards,
Andrej Golovnin

Re: 8154956: Module system implementation refresh (4/2016)

[Alan Bateman]

On 30/04/2016 20:49, Andrej Golovnin wrote:

:
I have a stupid question: Is it guaranteed that entry.getFileName().toString 
always
returns a value in lowercase?

For now, the implementation only treat files on the module path ending 
in ".jar" as candidate modular JAR or automatic modules. You will get an 
unrecognized module error with `java -modulepath foo.JAR ..` for 
example. You'll see the same in the tools. This has not changed in this 
refresh.


-Alan

Re: 8154956: Module system implementation refresh (4/2016)

[Andrej Golovnin]

Hi Claes,

> diff -r 930fcc6b74c8 
> src/java.base/share/classes/java/lang/module/ModulePath.java
> --- a/src/java.base/share/classes/java/lang/module/ModulePath.java Fri Apr 29 
> 18:47:24 2016 -0700
> +++ b/src/java.base/share/classes/java/lang/module/ModulePath.java Sat Apr 30 
> 16:03:03 2016 +0200
> @@ -267,8 +267,9 @@
> 
> if (attrs.isDirectory()) {
> return readExplodedModule(entry); // may return null
> -} if (attrs.isRegularFile()) {
> -String fn = entry.getFileName().toString();
> +}
> +String fn = entry.getFileName().toString();
> +if (attrs.isRegularFile()) {
> if (fn.endsWith(".jar")) {
> return readJar(entry);
> } else if (fn.endsWith(".jmod")) {
> @@ -279,7 +280,6 @@
> }
> 
> // skip hidden files
> -String fn = entry.getFileName().toString();
> if (fn.startsWith(".") || Files.isHidden(entry)) {
> return null;
> } else {

I have a stupid question: Is it guaranteed that entry.getFileName().toString 
always
returns a value in lowercase?

Best regards,
Andrej Golovnin

Re: 8154956: Module system implementation refresh (4/2016)

[Alan Bateman]

On 30/04/2016 15:58, Claes Redestad wrote:


String fn is potentially calculated twice and could be moved to 
directly after the first if (also gets rid of the odd looking "} if" 
construct):
The "odd looking } if" shouldn't be there, it's benign but fat fingered 
in the editor I assume. For hidden or not-a-module files then the file 
name is created twice but not really an issue. We can make it a bit more 
readable though so good to point this out.




:

On line 448 in the same file a Set (serviceNames) is created from a 
stream just to iterate over it and could be replaced by 
distinct().forEach(...)
Some of the files in META-INF/services may not be service configuration 
files so this is why toServiceName returns an optional.




:

hashCode should always store a non-zero value to hash to avoid 
recalculation in degenerate cases, e.g., if the calculated hash is 0, 
store -1.

Okay, I think this pattern is itself too.




http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/jdk.jartool/share/classes/sun/tools/jar/Main.java.udiff.html 

http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java.udiff.html 



This adds quite a bit of code duplication between these tools - 
transpose() etc - could (some of) this be refactored to a shared utility?


Yes, this is something that Mandy and I had a brief discussion recently 
and I think it's something for future cleanup.


-Alan

Re: 8154956: Module system implementation refresh (4/2016)

[Alan Bateman]

On 30/04/2016 05:48, Mandy Chung wrote:

:
I reviewed all repos except hotspot.  Looks good in general.  Minor comments:

Configuration.java
Nit: line 361-364 there is an extra space

Will fix.


sun/tools/jar/Main.java
   In the printModuleDescriptor method, creating a List from a stream is not 
necessary.
   1668 md.requires().stream().sorted().collect(toList()).forEach(

sortExports can be replaced as well.

I have a patch to clean this up that I can push to jake:
   http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jar-jmod-cleanup/

Go ahead, I have another small batch of clean-ups to get in too so I'll 
refresh the webrev on Monday.


-Alan

Re: 8154956: Module system implementation refresh (4/2016)

[Claes Redestad]

Lots of good changes here!

I've skimmed the changes and have some comments and minor things (feel 
free to ignore everything for this refresh):


http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/java.base/share/classes/java/lang/module/ModulePath.java.udiff.html

String fn is potentially calculated twice and could be moved to directly 
after the first if (also gets rid of the odd looking "} if" construct):


diff -r 930fcc6b74c8 
src/java.base/share/classes/java/lang/module/ModulePath.java
--- a/src/java.base/share/classes/java/lang/module/ModulePath.java Fri 
Apr 29 18:47:24 2016 -0700
+++ b/src/java.base/share/classes/java/lang/module/ModulePath.java Sat 
Apr 30 16:03:03 2016 +0200

@@ -267,8 +267,9 @@

 if (attrs.isDirectory()) {
 return readExplodedModule(entry); // may return null
-} if (attrs.isRegularFile()) {
-String fn = entry.getFileName().toString();
+}
+String fn = entry.getFileName().toString();
+if (attrs.isRegularFile()) {
 if (fn.endsWith(".jar")) {
 return readJar(entry);
 } else if (fn.endsWith(".jmod")) {
@@ -279,7 +280,6 @@
 }

 // skip hidden files
-String fn = entry.getFileName().toString();
 if (fn.startsWith(".") || Files.isHidden(entry)) {
 return null;
 } else {

On line 448 in the same file a Set (serviceNames) is created from a 
stream just to iterate over it and could be replaced by 
distinct().forEach(...)


http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/java.base/share/classes/java/lang/module/ModuleReference.java.udiff.html

hashCode should always store a non-zero value to hash to avoid 
recalculation in degenerate cases, e.g., if the calculated hash is 0, 
store -1.


http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/jdk.jartool/share/classes/sun/tools/jar/Main.java.udiff.html
http://cr.openjdk.java.net/~alanb/8154956/1/jdk/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java.udiff.html

This adds quite a bit of code duplication between these tools - 
transpose() etc - could (some of) this be refactored to a shared utility?


Thanks!

/Claes

On 2016-04-29 14:38, Alan Bateman wrote:


There have several changes in jake that we need to bring into 
jdk9/dev. The main changes are:


1. The policy described in JEP 261 for the root modules to resolve 
when compiling code in the unnamed module or where the main class is 
loaded from the class file. This is a disruptive change and we need to 
get through the transition.


Related is a new token `ALL-DEFAULT` that can be specified to 
`-addmods` to resolve the same roots when the initial module is a 
named module. This will eventually replace `ALL-SYSTEM` but we can't 
remove that just yet. The launchers for javac, javadoc, jlink and a 
few other tools that load arbitrary code in unnamed modules are now 
compiled with this option.


2. The transition to the new form of -Xpatch. This is mostly changes 
in hotspot but there are changes in other repos too to drop or replace 
the old form of -Xpatch (boot cycle builds for example).


3. Removal of the old form of -XaddExports and -XaddReads. This has 
build + test changes.


4. The second phase of integrity hashing. With this phase then the 
build records in java.base the hashes of the non-upgradeable modules. 
This prevents accidental linking of standard/JDK modules from 
different JDK builds. The jar and jmod tools have updated options to 
support this.


5. Peter Levart's patch to replace the internal WeakSet in jlr.Module 
with a WeakKeyPair.


6. Updates to jlink option handling. The reason this went into 
jdk9/dev is that it is disruptive to FX packager and it's hard to 
coordinate with FX when it's a separate forest. The packager class 
that Chris Bensen has added to jlink will allows us to iterate on 
jlink with reduced risk of breaking FX.


There are several small bug fixes and clean-ups in several areas, 
we'll have a lot more of these for the next update.


One other thing to mention is that we've bumped the required version 
of jtreg as jtreg relies on-Xpatch to add test cases into modules and 
so it needed to be updated too.


The webrevs, all repos are here:
  http://cr.openjdk.java.net/~alanb/8154956/1/

There are a couple of files in the webrevs that we probably won't 
bring to JDK 9 in this update:


i. We have a patch to IDL compiler in the corba repo that needs a more 
extensive fix.
ii. The javadoc change in ModuleFinder as there are still details to 
decide on how modular JARs work as multi-release JARs.


One other point is that the webrevs are against jdk-9+116 for now. 
I've done a test merge + build with the current jdk9/dev forest and 
there are only a few conflicts to fix. I will re-merge + test with 
jdk9/dev once we have agreed the changes for this update.


Finally, just to say that we'll probably continue in jake for a 

Re: 8154956: Module system implementation refresh (4/2016)

[Chris Hegarty]

> On 30 Apr 2016, at 05:48, Mandy Chung  wrote:
> 
> 
>> On Apr 29, 2016, at 5:38 AM, Alan Bateman  wrote:
>> 
>> The webrevs, all repos are here:
>> http://cr.openjdk.java.net/~alanb/8154956/1/
> 
> I reviewed all repos except hotspot.  Looks good in general.  Minor comments:
> 
> Configuration.java
>   Nit: line 361-364 there is an extra space
> 
> sun/tools/jar/Main.java
>  In the printModuleDescriptor method, creating a List from a stream is not 
> necessary.
>  1668 md.requires().stream().sorted().collect(toList()).forEach(
> 
> sortExports can be replaced as well.
> 
> I have a patch to clean this up that I can push to jake:
>  http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jar-jmod-cleanup/

Thank you Mandy,  I attempted to clean this up recently, but your further 
updates are much better.

-Chris.

Re: 8154956: Module system implementation refresh (4/2016)

[Mandy Chung]

> On Apr 29, 2016, at 5:38 AM, Alan Bateman  wrote:
> 
> The webrevs, all repos are here:
>  http://cr.openjdk.java.net/~alanb/8154956/1/

I reviewed all repos except hotspot.  Looks good in general.  Minor comments:

Configuration.java
   Nit: line 361-364 there is an extra space

sun/tools/jar/Main.java
  In the printModuleDescriptor method, creating a List from a stream is not 
necessary.
  1668 md.requires().stream().sorted().collect(toList()).forEach(

sortExports can be replaced as well.

I have a patch to clean this up that I can push to jake:
  http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jar-jmod-cleanup/

Mandy