Re: 8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

On 10/02/2017 10:00, Peter Levart wrote:



First, just a nit...

java.lang.module.Resolver:

 320 private void addFoundModule(ModuleReference mref) {
 321 ModuleDescriptor descriptor = mref.descriptor();
 322 nameToReference.put(descriptor.name(), mref);
 323
 324 if (descriptor.osName().isPresent()
 325 || descriptor.osArch().isPresent()
 326 || descriptor.osVersion().isPresent())
 327 checkTargetConstraints(descriptor);
 328 }

...perhaps more correct would be to reverse the order: 1st check 
target constraints and then add descriptor to map. Otherwise in case 
of check failure, you end up with a Resolver instance that is poisoned 
with incompatible module descriptor. Maybe you always throw away such 
Resolver if this method fails, but maybe someone will sometime try to 
recover and continue to use the Resolver for rest of modules?
Yes, fair point, it would be better to do this check first. I don't 
think we have any issue now because this is an internal class and the 
resolver is always thrown away after an exception. The ModuleTarget 
attribute isn't completely firmed up yet so I expect we will be back to 
this code soon.






...then something more involving...

java.lang.reflect.AccessibleObject::canAccess could share access cache 
with internal method checkAccess. In particular since one might use 
this new method in scenarios like:


Method method = ...;

if (method.canAccess(target)) {
method.invoke(target, ...);
} else {

or the other usage I would expect to see is:

  if (method.canAccess(target) || method.tryAccessible()) { .. }


...

Here's what you could do:

http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.01/ 

Good idea. Do you want to create an issue for this and follow-up on 
core-libs-dev? The changes are in jdk9/dev now so it can follow. A minor 
nit is that the InternalError was useful to catch bad changes, a NPE 
could work too but it should never happen.


-Alan

Re: 8173393: Module system implementation refresh (2/2017)

[Peter Levart]

Hi Alan,

On 02/09/2017 10:28 PM, Alan Bateman wrote:

On 07/02/2017 13:23, Alan Bateman wrote:

I will re-generate the webrevs later in the week once jdk-9+156 is 
promoted before eventually merging with jdk9/dev in advance of the 
eventual push.

I've sync'ed up jake to jdk-9+156 and re-generated the webrevs:
   http://cr.openjdk.java.net/~alanb/8173393/3/

Assuming that nothing significant comes up then these are the changes 
that I'd like to push to jdk9/dev for jdk-9+157.


-Alan


First, just a nit...

java.lang.module.Resolver:

 320 private void addFoundModule(ModuleReference mref) {
 321 ModuleDescriptor descriptor = mref.descriptor();
 322 nameToReference.put(descriptor.name(), mref);
 323
 324 if (descriptor.osName().isPresent()
 325 || descriptor.osArch().isPresent()
 326 || descriptor.osVersion().isPresent())
 327 checkTargetConstraints(descriptor);
 328 }

...perhaps more correct would be to reverse the order: 1st check target 
constraints and then add descriptor to map. Otherwise in case of check 
failure, you end up with a Resolver instance that is poisoned with 
incompatible module descriptor. Maybe you always throw away such 
Resolver if this method fails, but maybe someone will sometime try to 
recover and continue to use the Resolver for rest of modules?



...then something more involving...

java.lang.reflect.AccessibleObject::canAccess could share access cache 
with internal method checkAccess. In particular since one might use this 
new method in scenarios like:


Method method = ...;

if (method.canAccess(target)) {
method.invoke(target, ...);
} else {
...

Here's what you could do:

http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.01/


Regards, Peter

Re: 8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

On 07/02/2017 13:23, Alan Bateman wrote:

I will re-generate the webrevs later in the week once jdk-9+156 is 
promoted before eventually merging with jdk9/dev in advance of the 
eventual push.

I've sync'ed up jake to jdk-9+156 and re-generated the webrevs:
   http://cr.openjdk.java.net/~alanb/8173393/3/

Assuming that nothing significant comes up then these are the changes 
that I'd like to push to jdk9/dev for jdk-9+157.


-Alan

Re: 8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

On 08/02/2017 03:10, Paul Sandoz wrote:


Hi,

Just minor stuff in the JDK area (i browsed quickly through the tests).

Thanks, a few comments below.



Paul.


java.lang.module.Configuration
—

  321  * @implNote In the implementation then observability of modules may 
depend

s/then/the
"then" should be okay, it could also be "then the" (that might be what 
you mean).






:

Potential optimisation. Convert the sets to arrays, sort ‘em, then use 
Arrays.compare.
Yes, that could work. It's a real corner case to have to do this but we 
can do better for such cases.





BuiltinClassLoader
—

  925 private ModuleReader moduleReaderFor(ModuleReference mref) {
  926 return moduleToReader.computeIfAbsent(mref, m -> 
createModuleReader(m));
  927 }

Use this:: createModuleReader
I'll defer to Claes on this, mostly because #classes triggered to load 
here is observable in startup tests.





jdk.internal.module.Builder
—

  279 Set modifiers;
  280 if (open || synthetic) {
  281 modifiers = new HashSet<>();
  282 if (open) modifiers.add(ModuleDescriptor.Modifier.OPEN);
  283 if (synthetic) 
modifiers.add(ModuleDescriptor.Modifier.SYNTHETIC);
  284 modifiers = Collections.unmodifiableSet(modifiers);
  285 } else {
  286 modifiers = Collections.emptySet();
  287 }

It would be nice to use the small collection methods given the sizes. The 
following might work:
This is jlink plugin and would be really odd to have modules with the 
synthetic modifier. There are no open modules in the JDK images but a 
custom image may include some so point taken, this could be more efficient.




:


test/tools/jar/modularJar/Basic.java
—

  331 @Test(enabled = false)
  332 public void partialUpdateFooMainClass() throws IOException {

Just checking if that test still needs to be disabled.
There are fixes going into jdk9/dev for the jar tool that should allow 
us to re-enable this, it all depends on when the changes will meet up. 
If we have to push with that test disabled then I'll make sure there is 
a bug.




:

—

Separately, we missed the opportunity to add lexicographical comparison and 
mismatch to List (we added them for arrays).


Yes.

-Alan.

Re: 8173393: Module system implementation refresh (2/2017)

[Lois Foltan]

Hotspot changes look good to me.
Lois

On 2/7/2017 8:23 AM, Alan Bateman wrote:
We've been accumulating changes in the jake forest for the last few 
weeks and it's time to bring the changes to jdk9/dev, to make 
jdk-9+157 if possible. JDK 9 is the first phase of rampdown and so 
this update will need to get approval via the FC-extension process.


The changes this time are significantly smaller than previous updates. 
Much of this update is API and javadoc cleanup. There are a few new 
methods, and a few methods have been renamed/changed, but it's 
otherwise small potatoes.


The webrevs with the changes are here:
   http://cr.openjdk.java.net/~alanb/8173393/1/

Note that the webrevs are against jdk-9+155 because that is what jake 
is currently at. I will re-generate the webrevs later in the week once 
jdk-9+156 is promoted before eventually merging with jdk9/dev in 
advance of the eventual push.


One other thing to note is that the hotspot+jdk repos have the changes 
for JDK-8171855, this is only because that change was backed up in 
jdk9/hs for several weeks and only got to jdk9/dev for jdk-9+156. This 
means that the only non-test change in the hotspot repo is to 
methodHandles.cpp.


In the langtools repo then most of the changes are mechanical, with 
the only real update being cleanup to jdeps and the change to javac + 
javap to use the right values for the requires flags (the issue that 
Rémi brought up on jigsaw-dev last week when sync'ing up ASM).


In the jdk repo then ignore the DEBUG_ADD_OPENS changes in 
ModuleBootstrap, that is not intended for JDK 9.


There are a few small bug fixes, and a few more startup improvements 
from Claes. There are a small number of tests that aren't in jake yet 
for this update but they should be before I refresh the webrev later 
in the week.


-Alan

Re: 8173393: Module system implementation refresh (2/2017)

[Daniel Fuchs]

Hi Alan,

jaxp and stack walking class (and test) changes look OK to me.

best regards,

-- daniel


On 07/02/17 13:23, Alan Bateman wrote:

We've been accumulating changes in the jake forest for the last few
weeks and it's time to bring the changes to jdk9/dev, to make jdk-9+157
if possible. JDK 9 is the first phase of rampdown and so this update
will need to get approval via the FC-extension process.

The changes this time are significantly smaller than previous updates.
Much of this update is API and javadoc cleanup. There are a few new
methods, and a few methods have been renamed/changed, but it's otherwise
small potatoes.

The webrevs with the changes are here:
   http://cr.openjdk.java.net/~alanb/8173393/1/

Note that the webrevs are against jdk-9+155 because that is what jake is
currently at. I will re-generate the webrevs later in the week once
jdk-9+156 is promoted before eventually merging with jdk9/dev in advance
of the eventual push.

One other thing to note is that the hotspot+jdk repos have the changes
for JDK-8171855, this is only because that change was backed up in
jdk9/hs for several weeks and only got to jdk9/dev for jdk-9+156. This
means that the only non-test change in the hotspot repo is to
methodHandles.cpp.

In the langtools repo then most of the changes are mechanical, with the
only real update being cleanup to jdeps and the change to javac + javap
to use the right values for the requires flags (the issue that Rémi
brought up on jigsaw-dev last week when sync'ing up ASM).

In the jdk repo then ignore the DEBUG_ADD_OPENS changes in
ModuleBootstrap, that is not intended for JDK 9.

There are a few small bug fixes, and a few more startup improvements
from Claes. There are a small number of tests that aren't in jake yet
for this update but they should be before I refresh the webrev later in
the week.

-Alan

Re: 8173393: Module system implementation refresh (2/2017)

[Maurizio Cimadamore]

Langtools changes look good to me.

Maurizio


On 07/02/17 13:23, Alan Bateman wrote:
We've been accumulating changes in the jake forest for the last few 
weeks and it's time to bring the changes to jdk9/dev, to make 
jdk-9+157 if possible. JDK 9 is the first phase of rampdown and so 
this update will need to get approval via the FC-extension process.


The changes this time are significantly smaller than previous updates. 
Much of this update is API and javadoc cleanup. There are a few new 
methods, and a few methods have been renamed/changed, but it's 
otherwise small potatoes.


The webrevs with the changes are here:
   http://cr.openjdk.java.net/~alanb/8173393/1/

Note that the webrevs are against jdk-9+155 because that is what jake 
is currently at. I will re-generate the webrevs later in the week once 
jdk-9+156 is promoted before eventually merging with jdk9/dev in 
advance of the eventual push.


One other thing to note is that the hotspot+jdk repos have the changes 
for JDK-8171855, this is only because that change was backed up in 
jdk9/hs for several weeks and only got to jdk9/dev for jdk-9+156. This 
means that the only non-test change in the hotspot repo is to 
methodHandles.cpp.


In the langtools repo then most of the changes are mechanical, with 
the only real update being cleanup to jdeps and the change to javac + 
javap to use the right values for the requires flags (the issue that 
Rémi brought up on jigsaw-dev last week when sync'ing up ASM).


In the jdk repo then ignore the DEBUG_ADD_OPENS changes in 
ModuleBootstrap, that is not intended for JDK 9.


There are a few small bug fixes, and a few more startup improvements 
from Claes. There are a small number of tests that aren't in jake yet 
for this update but they should be before I refresh the webrev later 
in the week.


-Alan

8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

We've been accumulating changes in the jake forest for the last few 
weeks and it's time to bring the changes to jdk9/dev, to make jdk-9+157 
if possible. JDK 9 is the first phase of rampdown and so this update 
will need to get approval via the FC-extension process.


The changes this time are significantly smaller than previous updates. 
Much of this update is API and javadoc cleanup. There are a few new 
methods, and a few methods have been renamed/changed, but it's otherwise 
small potatoes.


The webrevs with the changes are here:
   http://cr.openjdk.java.net/~alanb/8173393/1/

Note that the webrevs are against jdk-9+155 because that is what jake is 
currently at. I will re-generate the webrevs later in the week once 
jdk-9+156 is promoted before eventually merging with jdk9/dev in advance 
of the eventual push.


One other thing to note is that the hotspot+jdk repos have the changes 
for JDK-8171855, this is only because that change was backed up in 
jdk9/hs for several weeks and only got to jdk9/dev for jdk-9+156. This 
means that the only non-test change in the hotspot repo is to 
methodHandles.cpp.


In the langtools repo then most of the changes are mechanical, with the 
only real update being cleanup to jdeps and the change to javac + javap 
to use the right values for the requires flags (the issue that Rémi 
brought up on jigsaw-dev last week when sync'ing up ASM).


In the jdk repo then ignore the DEBUG_ADD_OPENS changes in 
ModuleBootstrap, that is not intended for JDK 9.


There are a few small bug fixes, and a few more startup improvements 
from Claes. There are a small number of tests that aren't in jake yet 
for this update but they should be before I refresh the webrev later in 
the week.


-Alan