Re: 8170987: Module system implementation refresh (12/2016)

[Andrey Nazarov]

Langtools changes look good.

—Andrey
> On 15 Dec 2016, at 00:46, Alan Bateman  wrote:
> 
> Folks on jigsaw-dev will be aware that we are on yet another mission to bring 
> the changes accumulated in the jake forest to jdk9/dev. The plan this time is 
> to bring the changes to jdk9/dev to make jdk-9+150.
> 
> The changes in this update are mostly for JSR 376 issues 
> #VersionedDependences and #ModuleNameCharacters and so involve updates to the 
> binary form of the module declaration. There is also some small changes left 
> over from #IndirectQualifiedReflectiveAccess that we didn't include in the 
> last refresh.
> 
> This update has the implementation of Incubator Modules (JEP 11 [1]), 
> everything except the javac support. This was initially planned to push to 
> jdk9/dev but was re-routed to jake to avoid needing re-work when merged with 
> the changes in jake.
> 
> There is a bit of refactoring in the implementation in this update. We expect 
> to do more on than, plus lots of clean-up, once all the feature work is out 
> of way.
> 
> The webrevs with the changes for this update are here:
> 
>   http://cr.openjdk.java.net/~alanb/8170987/1
> 
> They are currently based on jdk-9+148 and will be re-based for jdk9/dev later 
> this week.
> 
> One review note this time is to ignore the changes in ModuleBootstrap for 
> DEBUG_ADD_OPENS, that is the only change in this webrev that is not proposed 
> to move to jdk9/dev.
> 
> -Alan
> 
> [1] http://openjdk.java.net/jeps/11
> 

Re: 8170987: Module system implementation refresh (12/2016)

[Mandy Chung]

> On Dec 15, 2016, at 10:13 AM, Alan Bateman  wrote:
> 
> On 15/12/2016 14:13, Claes Redestad wrote:
> 
>> 
>> The context here, I assume, is the increased startup cost to initialize
>> java.util.regex in 9 (and a few regression fixes related to this that
>> I've done in the area which may have involved avoiding adding a
>> regex-free fast path for trivial but common cases):
> Yes although it's not an issue at this time.. Looking at it again then we 
> should be able to decompose it at link time and generate code that 
> reconstitutes it from its parts. That would avoid needing to reparse at 
> startup.

That’s one possibility that we could consider in the future.

Mandy

Re: 8170987: Module system implementation refresh (12/2016)

[Alan Bateman]

On 15/12/2016 14:13, Claes Redestad wrote:



The context here, I assume, is the increased startup cost to initialize
java.util.regex in 9 (and a few regression fixes related to this that
I've done in the area which may have involved avoiding adding a
regex-free fast path for trivial but common cases):
Yes although it's not an issue at this time.. Looking at it again then 
we should be able to decompose it at link time and generate code that 
reconstitutes it from its parts. That would avoid needing to reparse at 
startup.


-Alan

Re: 8170987: Module system implementation refresh (12/2016)

[mark . reinhold]

2016/12/14 15:31:18 -0800, claes.redes...@oracle.com:
> ...
> 
> WARNING could be a local anonymous class inside
> printStackTraceIfExposedReflectively. ;-)

Good point -- fixed.

- Mark

Re: 8170987: Module system implementation refresh (12/2016)

[Chris Hegarty]

> On 15 Dec 2016, at 01:17, Mandy Chung  wrote:
> ...
> 
> src/java.base/share/classes/jdk/internal/module/ModuleResolution.java
> 
>  64 throw new RuntimeException("cannot add deprecated to " + 
> value);
> 
> This comment applies to ModuleResoluton::with* methods.  This should
> probably be an InternalError?  

I think InternalError is suitable here. These checks are to ensure tools don’t 
do
anything inappropriate.

> 108 return String.valueOf(value);
> 
> Nit: since you override toString method, might be helpful to print
> an informative description.  

Done.

> src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
> 
> 1102 if (value.equals("deprecated"))
> 1103 return (new ModuleResolution(0)).withDeprecated();
> 1104 else if (value.equals("deprecated-for-removal"))
> 1105 return (new 
> ModuleResolution(0)).withDeprecatedForRemoval();
> 1106 else if (value.equals("incubating"))
> 1107 return (new ModuleResolution(0)).withIncubating();
> 
> Why not passing the flag to ModuleResolution constructor?  Similar
> statement is also in sun/tools/jar/GNUStyleOptions.java.

I cleaned this up a little. I don’t want the tools to have knowledge of the 
actual 
flag values.

> I was wondering if jmod describe and jar —-print-module-descriptor should
> print all optional attributes.  While the module resolution is of limited
> use, it would be handy to print all optional attributes, if present rather
> than having to run java.

Agreed.

> It’s okay to follow up as a separate JBS issue if we want to do that.

If I don’t get to it before this Friday, I’ll follow up with a separate issue.

> test/jdk/modules/incubator/ImageModules.java
>  @modules jdk.jlink jdk.jartool are missing.  I have fixed it.

Thanks.
-Chris.

Re: 8170987: Module system implementation refresh (12/2016)

[Maurizio Cimadamore]

Langtools changes look good - I like the changes in ClassReader/Writer.

Maurizio


On 14/12/16 21:46, Alan Bateman wrote:
Folks on jigsaw-dev will be aware that we are on yet another mission 
to bring the changes accumulated in the jake forest to jdk9/dev. The 
plan this time is to bring the changes to jdk9/dev to make jdk-9+150.


The changes in this update are mostly for JSR 376 issues 
#VersionedDependences and #ModuleNameCharacters and so involve updates 
to the binary form of the module declaration. There is also some small 
changes left over from #IndirectQualifiedReflectiveAccess that we 
didn't include in the last refresh.


This update has the implementation of Incubator Modules (JEP 11 [1]), 
everything except the javac support. This was initially planned to 
push to jdk9/dev but was re-routed to jake to avoid needing re-work 
when merged with the changes in jake.


There is a bit of refactoring in the implementation in this update. We 
expect to do more on than, plus lots of clean-up, once all the feature 
work is out of way.


The webrevs with the changes for this update are here:

   http://cr.openjdk.java.net/~alanb/8170987/1

They are currently based on jdk-9+148 and will be re-based for 
jdk9/dev later this week.


One review note this time is to ignore the changes in ModuleBootstrap 
for DEBUG_ADD_OPENS, that is the only change in this webrev that is 
not proposed to move to jdk9/dev.


-Alan

[1] http://openjdk.java.net/jeps/11

Re: 8170987: Module system implementation refresh (12/2016)

[Alan Bateman]

On 15/12/2016 01:17, Mandy Chung wrote:


I have pushed the change to rename jdk.crypto.pkcs11 and jdk.pack200
and dropped java.compact$N.  So module-info.java changes will not be
needed when you sync with jdk9/dev.

Thank you. I'll do a merge today to see that everything works together.


:

Not sure if it’s intended to have the javadoc for isJavaIdentifier
method be the same as isBinaryName.
We can drop it but it was left there to avoid needing to change the 
usages that will be changing once we sort out residual issues in the 
Builder API, specifically the uses/provides methods that don't yet do 
the right validation (the `provides` methods shouldn't allow simple 
names for example, it also needs to ensure that the builder can't create 
a ModuleDescriptor that claim to have a provider that is not in the 
module. So I think this will all clean itself up in time.




When we use —-module-version for user modules, the runtime will load
regex.  The system modules jlink plugin uses the cached version if
JDK modules to be compiled with —0module-version in the future.
This might be something we should look at in the future for performance.
I'm sure Claes will be interested in that although I don't think we have 
any need to compile the JDK modules with --module-version, except maybe 
for testing exploded modules.




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

   64 throw new RuntimeException("cannot add deprecated to " + 
value);

This comment applies to ModuleResoluton::with* methods.  This should
probably be an InternalError?

IllegalArgumentException will probably work here.

-Alan

Re: 8170987: Module system implementation refresh (12/2016)

[Alan Bateman]

On 14/12/2016 23:31, Claes Redestad wrote:


Hi,

I took a quick pass over the jdk changes. It generally looks very good,
but I've got some comments:

MethodHandles.Lookup.dropLookupMode: The javadoc doesn't really roll
of the tongue here. Maybe "Creates a new lookup from the current one
where the given lookup mode has been dropped. ..." for starters?
John has a few tweaks to javadoc and also changes it to allow PROTECTED 
be dropped. I will get those changes into jake today and then see if we 
can improve the wording a bit.




ModuleDescriptor$Builder: should automatic be moved into a constructor
and automatic(boolean) removed for consistency with other boolean
attributes? My gut feeling tells me that
Builder.module("name").automatic(true) is non-sensical (not to mention
Builder.automaticModule("name").automatic(false)). It probably makes
no sense to export it through the JLMA bridge, but could avoid that
by adding a new private constructor called by the current.
We have re-visit a few things here as there are open questions on 
whether creating an automatic module via the Builder should require all 
packages to be exported and open. So I expect there will be changes here 
once we get to that overhaul.




WARNING could be a local anonymous class inside
printStackTraceIfExposedReflectively. ;-) A more noticeable cleanup
would be to move these methods to jdk/internal/reflect/Reflection.java
where there's now what appears to be code duplication (although the
printed messages diverge).
I'll look at it again, it was done this way to make it easy to leave the 
DEBUG_ADD_OPENS out.




In ClassWriter.java there's a comment line that seems to have been
removed by mistake.

Ugh, well spotted.

-Alan

Re: 8170987: Module system implementation refresh (12/2016)

[Mandy Chung]

> 
> The webrevs with the changes for this update are here:
> 
>   http://cr.openjdk.java.net/~alanb/8170987/1

I have pushed the change to rename jdk.crypto.pkcs11 and jdk.pack200
and dropped java.compact$N.  So module-info.java changes will not be 
needed when you sync with jdk9/dev.

I reviewed all changes except javac/javadoc changes.  Looks good in 
general. 

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

115 /**
116 * Returns {@code true} if the given name is a legal binary name.
117 */
118 public static boolean isJavaIdentifier(String name) {

Not sure if it’s intended to have the javadoc for isJavaIdentifier 
method be the same as isBinaryName.

When we use —-module-version for user modules, the runtime will load
regex.  The system modules jlink plugin uses the cached version if
JDK modules to be compiled with —0module-version in the future.
This might be something we should look at in the future for performance.

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

  64 throw new RuntimeException("cannot add deprecated to " + 
value);

This comment applies to ModuleResoluton::with* methods.  This should
probably be an InternalError?  

 108 return String.valueOf(value);

Nit: since you override toString method, might be helpful to print
an informative description.  

src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java

1102 if (value.equals("deprecated"))
1103 return (new ModuleResolution(0)).withDeprecated();
1104 else if (value.equals("deprecated-for-removal"))
1105 return (new 
ModuleResolution(0)).withDeprecatedForRemoval();
1106 else if (value.equals("incubating"))
1107 return (new ModuleResolution(0)).withIncubating();

Why not passing the flag to ModuleResolution constructor?  Similar
statement is also in sun/tools/jar/GNUStyleOptions.java.

I was wondering if jmod describe and jar —-print-module-descriptor should
print all optional attributes.  While the module resolution is of limited
use, it would be handy to print all optional attributes, if present rather
than having to run javap.

It’s okay to follow up as a separate JBS issue if we want to do that.

test/jdk/modules/incubator/ImageModules.java
  @modules jdk.jlink jdk.jartool are missing.  I have fixed it.

Mandy

Re: 8170987: Module system implementation refresh (12/2016)

[Claes Redestad]

Hi,

I took a quick pass over the jdk changes. It generally looks very good,
but I've got some comments:

MethodHandles.Lookup.dropLookupMode: The javadoc doesn't really roll
of the tongue here. Maybe "Creates a new lookup from the current one
where the given lookup mode has been dropped. ..." for starters?

ModuleDescriptor$Builder: should automatic be moved into a constructor
and automatic(boolean) removed for consistency with other boolean
attributes? My gut feeling tells me that
Builder.module("name").automatic(true) is non-sensical (not to mention
Builder.automaticModule("name").automatic(false)). It probably makes
no sense to export it through the JLMA bridge, but could avoid that
by adding a new private constructor called by the current.

WARNING could be a local anonymous class inside
printStackTraceIfExposedReflectively. ;-) A more noticeable cleanup
would be to move these methods to jdk/internal/reflect/Reflection.java
where there's now what appears to be code duplication (although the
printed messages diverge).

I see the Checks.isJavaIdentifier has been reworked, which should also
resolve the correctness issues we found here[1]. Good!

In ClassWriter.java there's a comment line that seems to have been
removed by mistake.

Thanks!

/Claes

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

On 2016-12-14 22:46, Alan Bateman wrote:

Folks on jigsaw-dev will be aware that we are on yet another mission to
bring the changes accumulated in the jake forest to jdk9/dev. The plan
this time is to bring the changes to jdk9/dev to make jdk-9+150.

The changes in this update are mostly for JSR 376 issues
#VersionedDependences and #ModuleNameCharacters and so involve updates
to the binary form of the module declaration. There is also some small
changes left over from #IndirectQualifiedReflectiveAccess that we didn't
include in the last refresh.

This update has the implementation of Incubator Modules (JEP 11 [1]),
everything except the javac support. This was initially planned to push
to jdk9/dev but was re-routed to jake to avoid needing re-work when
merged with the changes in jake.

There is a bit of refactoring in the implementation in this update. We
expect to do more on than, plus lots of clean-up, once all the feature
work is out of way.

The webrevs with the changes for this update are here:

   http://cr.openjdk.java.net/~alanb/8170987/1

They are currently based on jdk-9+148 and will be re-based for jdk9/dev
later this week.

One review note this time is to ignore the changes in ModuleBootstrap
for DEBUG_ADD_OPENS, that is the only change in this webrev that is not
proposed to move to jdk9/dev.

-Alan

[1] http://openjdk.java.net/jeps/11

Re: 8170987: Module system implementation refresh (12/2016)

[Lois Foltan]

On 12/14/2016 4:46 PM, Alan Bateman wrote:
Folks on jigsaw-dev will be aware that we are on yet another mission 
to bring the changes accumulated in the jake forest to jdk9/dev. The 
plan this time is to bring the changes to jdk9/dev to make jdk-9+150.


The changes in this update are mostly for JSR 376 issues 
#VersionedDependences and #ModuleNameCharacters and so involve updates 
to the binary form of the module declaration. There is also some small 
changes left over from #IndirectQualifiedReflectiveAccess that we 
didn't include in the last refresh.


This update has the implementation of Incubator Modules (JEP 11 [1]), 
everything except the javac support. This was initially planned to 
push to jdk9/dev but was re-routed to jake to avoid needing re-work 
when merged with the changes in jake.


There is a bit of refactoring in the implementation in this update. We 
expect to do more on than, plus lots of clean-up, once all the feature 
work is out of way.


The webrevs with the changes for this update are here:

   http://cr.openjdk.java.net/~alanb/8170987/1


I have reviewed the hotspot changes, looks good.
Lois



They are currently based on jdk-9+148 and will be re-based for 
jdk9/dev later this week.


One review note this time is to ignore the changes in ModuleBootstrap 
for DEBUG_ADD_OPENS, that is the only change in this webrev that is 
not proposed to move to jdk9/dev.


-Alan

[1] http://openjdk.java.net/jeps/11

8170987: Module system implementation refresh (12/2016)

[Alan Bateman]

Folks on jigsaw-dev will be aware that we are on yet another mission to 
bring the changes accumulated in the jake forest to jdk9/dev. The plan 
this time is to bring the changes to jdk9/dev to make jdk-9+150.


The changes in this update are mostly for JSR 376 issues 
#VersionedDependences and #ModuleNameCharacters and so involve updates 
to the binary form of the module declaration. There is also some small 
changes left over from #IndirectQualifiedReflectiveAccess that we didn't 
include in the last refresh.


This update has the implementation of Incubator Modules (JEP 11 [1]), 
everything except the javac support. This was initially planned to push 
to jdk9/dev but was re-routed to jake to avoid needing re-work when 
merged with the changes in jake.


There is a bit of refactoring in the implementation in this update. We 
expect to do more on than, plus lots of clean-up, once all the feature 
work is out of way.


The webrevs with the changes for this update are here:

   http://cr.openjdk.java.net/~alanb/8170987/1

They are currently based on jdk-9+148 and will be re-based for jdk9/dev 
later this week.


One review note this time is to ignore the changes in ModuleBootstrap 
for DEBUG_ADD_OPENS, that is the only change in this webrev that is not 
proposed to move to jdk9/dev.


-Alan

[1] http://openjdk.java.net/jeps/11