8173393: Module system implementation refresh (2/2017)

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
In reply to this post by Alan Bateman
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
>

Reply | Threaded
Open this post in threaded view
|

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

Lois Foltan
In reply to this post by Alan Bateman
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
>

Reply | Threaded
Open this post in threaded view
|

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

Paul Sandoz
Hi,

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

Paul.


java.lang.module.Configuration


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

s/then/the


ModuleDescriptor


2662     private static <T extends Object & Comparable<? super T>>
2663     int compare(Set<T> s1, Set<T> s2) {
2664         Iterator<T> iterator1 = new TreeSet<>(s1).iterator();
2665         Iterator<T> iterator2 =  new TreeSet<>(s2).iterator();
2666         while (iterator1.hasNext()) {
2667             if (!iterator2.hasNext())
2668                 return 1; // s1 has more elements
2669             T e1 = iterator1.next();
2670             T e2 = iterator2.next();
2671             int c = e1.compareTo(e2);
2672             if (c != 0)
2673                 return c;
2674         }
2675         if (iterator2.hasNext()) {
2676             return -1;  // s2 has more elements
2677         } else {
2678             return 0;
2679         }
2680     }

Potential optimisation. Convert the sets to arrays, sort ‘em, then use Arrays.compare.


BuiltinClassLoader


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

Use this:: createModuleReader


jdk.internal.module.Builder


 279         Set<ModuleDescriptor.Modifier> 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:

  Set<ModuleDescriptor.Modifier> modifiers;
  int n = (open ? 1 : 0) + (synthetic ? 1 : 0);
  if (n > 0) {
      ModuleDescriptor.Modifier[] ms = new ModuleDescriptor.Modifier[n];
      if (open)
          ms[—n] = ModuleDescriptor.Modifier.OPEN;
      if (synthetic)
          ms[—n] = ModuleDescriptor.Modifier.SYNTHETIC;
      modifiers = Set.of(ms);
  } else {
      modifiers = Set.of();
  }


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.





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

Paul.

> On 7 Feb 2017, at 14:48, Lois Foltan <[hidden email]> wrote:
>
> 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
>>
>


signature.asc (858 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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<ModuleDescriptor.Modifier> 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.
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
Hi,

On 2017-02-08 12:37, Alan Bateman wrote:

>>
>>
>> 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.

 From a startup perspective all alternatives are more or less equal
here, and generally speaking a lambda and a method reference are equal
as long as they're both non-capturing.

A detail I had missed here, though, is that the createModuleReader
method could be made static to ensure the lambda is actually
non-capturing (otherwise it'll unnecessarily allocate an object
preserving this).

So I'd make createModuleReader static and then pick and choose between
m -> createModuleReader(m) and BuiltinClassLoader::createModuleReader

Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman


On 08/02/2017 14:49, Claes Redestad wrote:
>
> From a startup perspective all alternatives are more or less equal
> here, and generally speaking a lambda and a method reference are equal
> as long as they're both non-capturing.
>
> A detail I had missed here, though, is that the createModuleReader
> method could be made static to ensure the lambda is actually
> non-capturing (otherwise it'll unnecessarily allocate an object
> preserving this).
Yes, good point, this can of course be static.

-Alan
Reply | Threaded
Open this post in threaded view
|

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

Paul Sandoz
In reply to this post by Alan Bateman

> On 8 Feb 2017, at 03:37, Alan Bateman <[hidden email]> wrote:
>
> 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).
>
Given the way i recall JavaDoc renders impl notes they tend to stand on their own rather as sections than be dependent on the flow of paragraphs previous to them.

Paul.

signature.asc (858 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Alan Bateman
I reviewed your updated webrev:
  http://cr.openjdk.java.net/~alanb/8173393/2

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/Module.java
 285             m.descriptor.packages().forEach(builder::opens);

This is not needed.  I took it out.

We probably will do another pass on the APIs to tag with @spec JPMS.  IllegalCallerException and the following in MethodHandles don’t have the tag.

 667         public static final int UNCONDITIONAL = PACKAGE << 2;
 841         public Lookup dropLookupMode(int modeToDrop) {

IIRC no specific spec change to ResourceBundle.Control.toBundleName. So @revised and @spec can be dropped.

Otherwise looks fine to me. I reviewed changes in jdk.jdeps in langtools and all other repos.

Mandy

Reply | Threaded
Open this post in threaded view
|

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

Karen Kinnear
In reply to this post by Alan Bateman
Hotspot changes look good.

thanks,
Karen

> On Feb 7, 2017, at 8:23 AM, Alan Bateman <[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
In reply to this post by 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
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Peter Levart


On 02/10/2017 02:49 PM, Alan Bateman wrote:
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

Ok, will create an issue and a follow-up change.

Regards, Peter