Re: Code review for jigsaw/jake -> jdk9/dev sync up
Hi Alan,
Overall this looks very good. I noticed a couple of nits...
1. I wonder if the new ServiceLoader API signature should be tweaked a
bit... There is a new method in ServiceLoader with the following
signature:
public Stream> stream()
...where Provider declares the following two methods:
Class type();
S get();
Which means that I can do the following:
ServiceLoader loader = ServiceLoader.load(MyService.class);
Class clazz =
loader.stream().map(Provider::type).findFirst().get();
But 'clazz' now holds a Class object representing the implementation
class (MyServiceImpl for example) not the MyService class/interface.
Could this be characterized as heap pollution? For example, I can not do
the following easily without unchecked casts:
Class clazz = MyServiceImpl.class;
Error: java: incompatible types: java.lang.Class
cannot be converted to java.lang.Class
So should Provider rather declare the following?
Class type();
Or alternatively, should ServiceLoader rather declare the following?
public Stream> stream();
2. In Resolver::makeGraph, the initial 'g2' graph from parent
configurations' "require transitive" is build this way:
for (Configuration parent : parents) {
if (parent != Configuration.empty()) {
parent.configurations().forEach(c -> {
c.modules().forEach(m1 -> {
m1.descriptor().requires().stream()
.filter(r ->
r.modifiers().contains(Modifier.TRANSITIVE))
.forEach(r -> {
String dn = r.name();
assert c.findModule(dn).isPresent() ||
r.modifiers().contains(Modifier.STATIC);
c.findModule(dn).ifPresent(m2 -> {
g2.computeIfAbsent(m1, k -> new
HashSet<>()).add(m2);
});
});
});
});
}
}
Now that there can be multiple parents of some configuration, those
parents can have common parents, but since you iterate the transitive
configurations of each individual parent in isolation, you can encounter
a common "diamond" configuration multiple times. Logically all is well
since you collect the required modules into Sets so multiplicity is
taken care of. But this is not very optimal from algorithm point. It
would be better to 1st make a stream of unique configurations of all
parents:
parents
.stream()
.flatMap(Configuration::configurations)
.distinct()
...and then continute processing this stream further. You could even
make this code more "functional" by using flatMap instead of forEach...
Map> g2 = parents
.stream()
.flatMap(Configuration::configurations)
.distinct() // optimization in case of common "diamond"
parent(s)
.flatMap(c ->
c.modules().stream().flatMap(m1 ->
m1.descriptor().requires().stream()
.filter(req ->
req.modifiers().contains(Modifier.TRANSITIVE))
.flatMap(req -> {
Optional m2 =
c.findModule(req.name());
assert m2.isPresent() ||
req.modifiers().contains(Modifier.STATIC);
return m2.stream();
})
.map(m2 -> Map.entry(m1, m2))
)
)
.collect(Collectors.groupingBy(
Map.Entry::getKey,
() -> new HashMap<>(capacity),
Collectors.mapping(Map.Entry::getValue, Collectors.toSet())
));
Regards, Peter
On 11/24/2016 04:25 PM, Alan Bateman wrote:
Folks on jigsaw-dev will know that we are on a mission to bring the
changes accumulated in the jake forest to jdk9/dev. We can think of
this as a refresh of the module system in JDK 9, the last big refresh
was in May with many small updates since then.
The focus this time is to bring the changes that are tied to JSR
issues into jdk9/dev, specifically the issues that are tracked on the
JSR issues list [1] as:
#CompileTimeDependences
#AddExportsInManifest
#ClassFileModuleName
#ClassFileAccPublic
#ServiceLoaderEnhancements
#ResourceEncapsulation/#ClassFilesAsResources
#ReflectiveAccessToNonExportedTypes
#AwkwardStrongEncapsulation
#ReadabilityAddedByLayerCreator
#IndirectQualifiedReflectiveAccess (partial)
#VersionsInModuleNames
#NonHierarchicalLayers
#ModuleAnnotations/#ModuleDeprecation
#ReflectiveAccessByInstrumentationAgents
Some of these issues are not "Resolved" yet, meaning there is still
ongoing discussion on the EG mailing list. That is okay, there is
nothing final here. If there are changes to these proposals then the
implementation changes will follow. Also, as I said in a mail to
jigsaw-dev yesterda
Re: Code review for jigsaw/jake -> jdk9/dev sync up
On 27/11/2016 11:15, Peter Levart wrote: Hi Alan, Overall this looks very good. I noticed a couple of nits... Thanks for going through the changes. : So should Provider rather declare the following? Class type(); Or alternatively, should ServiceLoader rather declare the following? public Stream> stream(); Well spotted, this should be Class. We could change the return of stream too but that might be more awkward to work with. : Now that there can be multiple parents of some configuration, those parents can have common parents, but since you iterate the transitive configurations of each individual parent in isolation, you can encounter a common "diamond" configuration multiple times. Logically all is well since you collect the required modules into Sets so multiplicity is taken care of. But this is not very optimal from algorithm point. It would be better to 1st make a stream of unique configurations of all parents: Fair comment, we haven't done any performance work for the multi parent scenario yet, mostly because #NonHierarchicalLayers is niche and there are still some open questions on how parents should be searched. So we can change this as you suggest except that it regresses startup, this is the reason for skipping Configuration.empty(). However we support both so I'll get that change in so that the more elegant code is used when creating dynamic configurations. -Alan
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? * ClassReader: should we have checks regarding an open module containing no open directives in the classfile? This seems to be called out in the spec [1] - see section 2.2 * At some point we should investigate better sharing strategy between ClassReader and ModuleNameReader * Names.dynamic seems unused * I note that the classfile attribute name changes are not captured in the spec (but I might be referring to a slightly older version). Maurizio [1] - http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.2 On 24/11/16 15:25, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field in java.util.TreeMap then running with `--add-opens java.base/java.util=ALL-UNNAMED` will keep that code working. A few miscellaneous notes for those that are reviewing: 1. We have some temporary/transition code in the top-level repo to deal with the importing of the JavaFX modules. This will be removed once the changes are in JDK 9 for the OpenJFX project to use. 2. In the jdk repo then it's important to understand that the module system is initialized at startup and there are many places where we need to keep startup performance in mind. This sometimes means less ele
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Hi Alan, I have reviewed the hotspot changes and they look good. Minor nit, src/share/vm/classfile/javaClasses.cpp only differs by the addition of a blank line. Thanks, Lois On 11/24/2016 10:25 AM, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field in java.util.TreeMap then running with `--add-opens java.base/java.util=ALL-UNNAMED` will keep that code working. A few miscellaneous notes for those that are reviewing: 1. We have some temporary/transition code in the top-level repo to deal with the importing of the JavaFX modules. This will be removed once the changes are in JDK 9 for the OpenJFX project to use. 2. In the jdk repo then it's important to understand that the module system is initialized at startup and there are many places where we need to keep startup performance in mind. This sometimes means less elegant code than might be used if startup wasn't such a big concern. 3. The changes in the jaxws repo make use of new APIs that means the code doesn't compile with JDK 7 or JDK 8. Our intention is to work with the JAXB and JAX-WS maintainers to address the issues in the upstream project and then bring those changes into jdk9/dev to replace the patches that we are forced to push for the short term. 4. You will see several tests where the value of the @modules tag has `:open` or `:+open`. This is new jtreg speak. The former mea
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Alan, I reviewed all the hotspot runtime changes - except the tests (Christian will review those) - and jvmti - which Dmitry Samersoff will review. They look good. thanks, Karen > On Nov 28, 2016, at 9:32 AM, Lois Foltan wrote: > > Hi Alan, > > I have reviewed the hotspot changes and they look good. Minor nit, > src/share/vm/classfile/javaClasses.cpp only differs by the addition of a > blank line. > > Thanks, > Lois > > On 11/24/2016 10:25 AM, Alan Bateman wrote: >> Folks on jigsaw-dev will know that we are on a mission to bring the changes >> accumulated in the jake forest to jdk9/dev. We can think of this as a >> refresh of the module system in JDK 9, the last big refresh was in May with >> many small updates since then. >> >> The focus this time is to bring the changes that are tied to JSR issues into >> jdk9/dev, specifically the issues that are tracked on the JSR issues list >> [1] as: >> >> #CompileTimeDependences >> #AddExportsInManifest >> #ClassFileModuleName >> #ClassFileAccPublic >> #ServiceLoaderEnhancements >> #ResourceEncapsulation/#ClassFilesAsResources >> #ReflectiveAccessToNonExportedTypes >> #AwkwardStrongEncapsulation >> #ReadabilityAddedByLayerCreator >> #IndirectQualifiedReflectiveAccess (partial) >> #VersionsInModuleNames >> #NonHierarchicalLayers >> #ModuleAnnotations/#ModuleDeprecation >> #ReflectiveAccessByInstrumentationAgents >> >> Some of these issues are not "Resolved" yet, meaning there is still ongoing >> discussion on the EG mailing list. That is okay, there is nothing final >> here. If there are changes to these proposals then the implementation >> changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], >> is that we will keep the jake forest open for ongoing prototyping and >> iteration, also ongoing implementation improvements where iteration or bake >> time is important. >> >> For the code review then the focus is therefore on sanity checking the >> changes that we would like to bring into jdk9/dev. We will not use this >> review thread to debate alternative designs or other big implementation >> changes that are more appropriate to bake in jake. >> >> To get going, I've put the webrevs with a snapshot of the changes in jake >> here: >>http://cr.openjdk.java.net/~alanb/8169069/0/ >> >> The changes are currently sync'ed against jdk-9+146 and will be rebased (and >> re-tested) against jdk9/dev prior to integration. There are a number of >> small changes that need to be added to this in the coming days, I will >> refresh the webrev every few days to take account of these updates. >> >> >> A few important points to mention, even if you aren't reviewing the changes: >> >> 1. This refresh requires a new version of jtreg to run the tests. The >> changes for this new version are in the code-tools/jtreg repository and the >> plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been >> added then we'll update the requiredVersion property in each TEST.ROOT to >> force everyone to update. >> >> 2. For developers trying out modules with the main line JDK 9 builds then be >> aware that `requires public` changes to `requires transitive` and the >> `provides` clause changes to require all providers for a specific service >> type to be in the same clause. Also be aware that the binary form of the >> module declaration (module-info.class) changes so you will need to recompile >> any modules. >> >> 3. Those running existing code on JDK 9 and ignoring modules will need to be >> aware of a disruptive change in this refresh. The disruptive change is >> #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it >> can't be used to break into non-public fields/methods of JDK classes. This >> change is going to expose a lot of hacks in existing code. We plan to send >> mail to jdk9-dev in advance of this integration to create awareness of this >> change. As per the original introduction of strong encapsulation then >> command line options (and now the manifest of application JAR files) can be >> used to keep existing code working. The new option is `--add-opens` to open >> a package in a module for deep reflection by other modules. As an example, >> if you find yourself with code that hacks into the private `comparator` >> field in java.util.TreeMap then running with `--add-opens >> java.base/java.util=ALL-UNNAMED` will keep that code working. >> >> >> A few miscellaneous notes for those that are reviewing: >> >> 1. We have some temporary/transition code in the top-level repo to deal with >> the importing of the JavaFX modules. This will be removed once the changes >> are in JDK 9 for the OpenJFX project to use. >> >> 2. In the jdk repo then it's important to understand that the module system >> is initialized at startup and there are many places where we need to keep >> startup performance in mind. This sometimes means less elegant code than >> might be used if star
Re: Code review for jigsaw/jake -> jdk9/dev sync up
On 24 Nov 2016, at 15:25, Alan Bateman wrote:
>
> ...
> To get going, I've put the webrevs with a snapshot of the changes in jake
> here:
>http://cr.openjdk.java.net/~alanb/8169069/0/
Overall this look very good. I ran through most of the changes in the jdk repo,
just a few small comments.
1) SuppressWarnings.java typo element - > elementS
38 * However, note that if a warning is suppressed in a {@code
39 * module-info} file, the suppression applies to elementS within the
40 * file and not to types contained within the module.
2) jartool Main.java
Maybe concealedPackages should have a comment about its use ( it is
used in the Validator, and not by Main at all ).
3) MethodHandles.java privateLookupIn
It might be clearer if the third bullet used {@code lookup}, or 'caller
lookup’, or ‘given lookup'?
The CALLER lookup has the {@link Lookup#MODULE MODULE} lookup mode.
4) ServiceLoader
{@code ExtendedCodecsFactory}
111 * will be treated as a provider factory and {@code
112 * ExtendedCodecsFactory.provider()} will be invoked to INSTANTIATE the
113 * provider.
Is 'instantiate' strictly true here? Should it simply be ‘return'
206 * If a named module declares more than one provider then the
providers
207 * are located in the order that they appear in the {@code provides}
table of
208 * the {@code Module} class file attribute ({@code
module-info.class}).
Wow. I assume the JLS, or otherwise, will specify that the order in which the
providers are listed in the module-info be preserved, no? Maybe this item
could
mention that. The class file reference can still be kept, but seems a bit
low-level
for developers.
-Chris.
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Reviewed Nashorn changes. All fine.
-Sundar
On 11/28/2016 8:17 PM, Chris Hegarty wrote:
> On 24 Nov 2016, at 15:25, Alan Bateman wrote:
>> ...
>> To get going, I've put the webrevs with a snapshot of the changes in jake
>> here:
>>http://cr.openjdk.java.net/~alanb/8169069/0/
> Overall this look very good. I ran through most of the changes in the jdk
> repo,
> just a few small comments.
>
> 1) SuppressWarnings.java typo element - > elementS
>
> 38 * However, note that if a warning is suppressed in a {@code
> 39 * module-info} file, the suppression applies to elementS within the
> 40 * file and not to types contained within the module.
>
> 2) jartool Main.java
> Maybe concealedPackages should have a comment about its use ( it is
> used in the Validator, and not by Main at all ).
>
> 3) MethodHandles.java privateLookupIn
> It might be clearer if the third bullet used {@code lookup}, or 'caller
> lookup’, or ‘given lookup'?
>
> The CALLER lookup has the {@link Lookup#MODULE MODULE} lookup mode.
>
> 4) ServiceLoader
> {@code ExtendedCodecsFactory}
> 111 * will be treated as a provider factory and {@code
> 112 * ExtendedCodecsFactory.provider()} will be invoked to INSTANTIATE the
> 113 * provider.
>
> Is 'instantiate' strictly true here? Should it simply be ‘return'
>
> 206 * If a named module declares more than one provider then the
> providers
> 207 * are located in the order that they appear in the {@code
> provides} table of
> 208 * the {@code Module} class file attribute ({@code
> module-info.class}).
>
> Wow. I assume the JLS, or otherwise, will specify that the order in which
> the
> providers are listed in the module-info be preserved, no? Maybe this item
> could
> mention that. The class file reference can still be kept, but seems a bit
> low-level
> for developers.
>
> -Chris.
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Thanks for the comments Maurizio. On 28.11.2016 12:28, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? It seemed to me that having two directive interfaces in the API for directives that have the same structure was unnecessary (as we don't have MethodElement and ConstructorElement, but just ExecutableElement, or TypeElement that represents a class, an interface, an annotation type or an enum type). If you think it would be better to have separate interfaces for exports and opens, I am OK with that as well. * ClassReader: should we have checks regarding an open module containing no open directives in the classfile? This seems to be called out in the spec [1] - see section 2.2 I think such checks would be fine, working on a patch. * At some point we should investigate better sharing strategy between ClassReader and ModuleNameReader * Names.dynamic seems unused Fixed: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/8156e205fcb4 Jan * I note that the classfile attribute name changes are not captured in the spec (but I might be referring to a slightly older version). Maurizio [1] - http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.2 On 24/11/16 15:25, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field
Re: Code review for jigsaw/jake -> jdk9/dev sync up
On 28/11/16 14:53, Jan Lahoda wrote: Thanks for the comments Maurizio. On 28.11.2016 12:28, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? It seemed to me that having two directive interfaces in the API for directives that have the same structure was unnecessary (as we don't have MethodElement and ConstructorElement, but just ExecutableElement, or TypeElement that represents a class, an interface, an annotation type or an enum type). If you think it would be better to have separate interfaces for exports and opens, I am OK with that as well. I agree that it would be redundant - perhaps the two directive can both share a common superclass which defines the common fields? I said that because it looks like for everything else, 'opens' and 'exports' are really two separate directives, with separate bytecode encodings and such (that is, the Module attribute has separate entries for 'opens' and 'exports'). * ClassReader: should we have checks regarding an open module containing no open directives in the classfile? This seems to be called out in the spec [1] - see section 2.2 I think such checks would be fine, working on a patch. ok * At some point we should investigate better sharing strategy between ClassReader and ModuleNameReader * Names.dynamic seems unused Fixed: http://hg.openjdk.java.net/jigsaw/jake/langtools/rev/8156e205fcb4 Thanks Maurizio Jan * I note that the classfile attribute name changes are not captured in the spec (but I might be referring to a slightly older version). Maurizio [1] - http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.2 On 24/11/16 15:25, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev
Re: Code review for jigsaw/jake -> jdk9/dev sync up
On 11/28/16 3:28 AM, Maurizio Cimadamore wrote: Hi, the langtools code looks generally ok. Few questions: * Why doesn't 'open' get its own directive in Directive.java - instead of relying on a 'mode' set on an export directive? I agree that "opens" leveraging a type named "Exports..." is particularly irksome. If we don't come up with a common supertype, I think we should clone the code. * ClassReader: should we have checks regarding an open module containing no open directives in the classfile? This seems to be called out in the spec [1] - see section 2.2 * At some point we should investigate better sharing strategy between ClassReader and ModuleNameReader Maybe, but ModuleNameReader is supposed to be a simple light weight class reader, just good enough to get the name. It is invoked from the file manager world, and so does not (should not) access anything to do with Symbols. If we eventually rewrite ClassReader to sit on top of a new lower-level abstraction, then maybe we can share code some more. * Names.dynamic seems unused * I note that the classfile attribute name changes are not captured in the spec (but I might be referring to a slightly older version). Maurizio [1] - http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.2 On 24/11/16 15:25, Alan Bateman wrote: Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field in java.util.Tree
Re: Code review for jigsaw/jake -> jdk9/dev sync up
For the JDK patch:
MethodHandles
—
176 public static Lookup privateLookupIn(Class targetClass, Lookup
lookup) throws IllegalAccessException {
177 SecurityManager sm = System.getSecurityManager();
178 if (sm != null) sm.checkPermission(ACCESS_PERMISSION);
179 if (targetClass.isPrimitive())
180 throw new IllegalArgumentException(targetClass + " is a
primitive class");
181 Module targetModule = targetClass.getModule();
182 Module callerModule = lookup.lookupClass().getModule();
183 if (callerModule != targetModule && targetModule.isNamed()) {
184 if (!callerModule.canRead(targetModule))
185 throw new IllegalAccessException(callerModule + " does not
read " + targetModule);
186 Class clazz = targetClass;
187 while (clazz.isArray()) {
188 clazz = clazz.getComponentType();
189 }
What happens if you pass a primitive array?
I think you need to specify what happens if an array class is passed and how
the target class is obtained, and an IAE if the "element type" of the array is
a primitive class.
(Separately i am looking forward to using this method to replace some Unsafe
usages between Thread and other classes within the base module.)
ModuleDescriptor
—
241 private Stream toStringStream(Set s) {
242 return s.stream().map(e -> e.toString().toLowerCase());
243 }
244
245 private String toString(Set mods, String what) {
246 return (Stream.concat(toStringStream(mods), Stream.of(what)))
247 .collect(Collectors.joining(" "));
248 }
The above occurs three times. Suggest moving to the enclosing class as package
private static methods.
Resolver
—
449 for (Configuration parent: parents) {
450 Optional om =
parent.findModule(dn);
451 if (om.isPresent()) {
452 other = om.get().reference();
453 break;
454 }
455 }
Replace with
ResolvedModule rm = findInParent(dm);
if (rm != null)
other = rm.reference();
532 Set requiresTransitive= new HashSet<>();
Formating glitch for “=“
ServiceLoader
—
1331 @Override
1332 @SuppressWarnings("unchecked")
1333 public boolean tryAdvance(Consumer> action) {
1334 if (ServiceLoader.this.reloadCount != expectedReloadCount)
1335 throw new ConcurrentModificationException();
1336 Provider next = null;
1337 if (index < loadedProviders.size()) {
1338 next = (Provider) loadedProviders.get(index);
1339 } else if (iterator.hasNext()) {
1340 next = iterator.next();
1341 } else {
1342 loadedAllProviders = true;
1343 }
1344 index++;
Move the index increment into the top if block, thereby it only gets increment
appropriately (no crazy overflow possible).
1353 @Override
1354 public int characteristics() {
1355 // need to decide on DISTINCT
1356 // not IMMUTABLE as structural interference possible
1357 return Spliterator.ORDERED + Spliterator.NONNULL;
1358 }
Should probably be consistent (subset of) with the characteristics reported for
loadedProviders.stream(), thus DISTINCT would not be appropriate in this case
unless you changed the optimal case of when all providers are loaded.
1552 public Optional findFirst() {
1553 Iterator iterator = iterator();
1554 if (iterator.hasNext()) {
1555 return Optional.of(iterator.next());
1556 } else {
1557 return Optional.empty();
1558 }
1559 }
return stream().findFirst() ?
Paul.
> On 24 Nov 2016, at 07:25, Alan Bateman wrote:
>
> Folks on jigsaw-dev will know that we are on a mission to bring the changes
> accumulated in the jake forest to jdk9/dev. We can think of this as a refresh
> of the module system in JDK 9, the last big refresh was in May with many
> small updates since then.
>
> The focus this time is to bring the changes that are tied to JSR issues into
> jdk9/dev, specifically the issues that are tracked on the JSR issues list [1]
> as:
>
> #CompileTimeDependences
> #AddExportsInManifest
> #ClassFileModuleName
> #ClassFileAccPublic
> #ServiceLoaderEnhancements
> #ResourceEncapsulation/#ClassFilesAsResources
> #ReflectiveAccessToNonExportedTypes
> #AwkwardStrongEncapsulation
> #ReadabilityAddedByLayerCreator
> #IndirectQualifiedReflectiveAccess (partial)
> #VersionsInModuleNames
> #NonHierarchicalLayers
> #ModuleAnnotations/#ModuleDeprecation
> #ReflectiveAccessByInstrumentationAgents
>
> Some of these issues are not "Resolved" yet, meaning there is still ongoing
> discussion on the EG mailing
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Thanks for going through the changes, a few comment/replies below.
On 28/11/2016 22:22, Paul Sandoz wrote:
:
What happens if you pass a primitive array?
I think you need to specify what happens if an array class is passed and how the target
class is obtained, and an IAE if the "element type" of the array is a primitive
class.
Yeah, it's something that I've been looking at too, mostly trying to
decide whether to reject all array types for now. It is possible to
specify an array class to Lookup::in but there are issues, it triggers
at least one one assert that needs attention.
(Separately i am looking forward to using this method to replace some Unsafe
usages between Thread and other classes within the base module.)
ModuleDescriptor
—
241 private Stream toStringStream(Set s) {
242 return s.stream().map(e -> e.toString().toLowerCase());
243 }
244
245 private String toString(Set mods, String what) {
246 return (Stream.concat(toStringStream(mods), Stream.of(what)))
247 .collect(Collectors.joining(" "));
248 }
The above occurs three times. Suggest moving to the enclosing class as package
private static methods.
Thanks, this is clean-up that wasn't done after going through several
iterations.
:}
Replace with
ResolvedModule rm = findInParent(dm);
if (rm != null)
other = rm.reference();
532 Set requiresTransitive= new HashSet<>();
Formating glitch for “=“
Thanks.
ServiceLoader
—
1331 @Override
1332 @SuppressWarnings("unchecked")
1333 public boolean tryAdvance(Consumer> action) {
1334 if (ServiceLoader.this.reloadCount != expectedReloadCount)
1335 throw new ConcurrentModificationException();
1336 Provider next = null;
1337 if (index < loadedProviders.size()) {
1338 next = (Provider) loadedProviders.get(index);
1339 } else if (iterator.hasNext()) {
1340 next = iterator.next();
1341 } else {
1342 loadedAllProviders = true;
1343 }
1344 index++;
Move the index increment into the top if block, thereby it only gets increment
appropriately (no crazy overflow possible).
I agree, this could cause a problem in some extreme scenario.
1353 @Override
1354 public int characteristics() {
1355 // need to decide on DISTINCT
1356 // not IMMUTABLE as structural interference possible
1357 return Spliterator.ORDERED + Spliterator.NONNULL;
1358 }
Should probably be consistent (subset of) with the characteristics reported for
loadedProviders.stream(), thus DISTINCT would not be appropriate in this case
unless you changed the optimal case of when all providers are loaded.
When all loaded then the characteristics come from ArrayList's
spliterator and so will be ORDERED|SIZED|SUBSIZED. For consistency then
we limit the above to ORDERED. I've been tempted to do more here but I
think we need to get some real world usage of this method to see if
anything is needed (esp. when the #providers is usually small).
:
1552 public Optional findFirst() {
1553 Iterator iterator = iterator();
1554 if (iterator.hasNext()) {
1555 return Optional.of(iterator.next());
1556 } else {
1557 return Optional.empty();
1558 }
1559 }
return stream().findFirst() ?
The stream elements are Provider rather than S so it could be
stream().findFirst().map(Provider::get). The reason it was based on
iterator is because it pre-dates stream but looking at it now then maybe
findFirst() should go away as it's trivial to do with the new stream method.
-Alan
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Hi,
I’ve reviewed Langtools code.
There are various comment “//TODO”, “//FIXME”, “//XXX”. I think they should be
revised. May be issues should be filed to track them.
Unused import at 37 import java.io.IOException; in
langtools/test/tools/javac/modules/ModuleInfoTest.java
ASCII graphics issue at 64 line in test/tools/javac/modules/GraphsTest.java
Unused builder method in langtools/test/tools/lib/toolbox/ModuleBuilder.java
159 public ModuleBuilder exportsDynamicPrivate(String pkg) {
141 public ModuleBuilder exportsDynamic(String pkg) {
179 public ModuleBuilder exportsDynamicTo(String pkg, String module) {
199 public ModuleBuilder exportsDynamicPrivateTo(String pkg, String module)
{
Javadoc is used instead of comment
langtools/test/tools/javac/modules/AnnotationsOnModules.java
/**
25 * @test
—Andrey
> On 24 Nov 2016, at 18:25, Alan Bateman wrote:
>
> Folks on jigsaw-dev will know that we are on a mission to bring the changes
> accumulated in the jake forest to jdk9/dev. We can think of this as a refresh
> of the module system in JDK 9, the last big refresh was in May with many
> small updates since then.
>
> The focus this time is to bring the changes that are tied to JSR issues into
> jdk9/dev, specifically the issues that are tracked on the JSR issues list [1]
> as:
>
> #CompileTimeDependences
> #AddExportsInManifest
> #ClassFileModuleName
> #ClassFileAccPublic
> #ServiceLoaderEnhancements
> #ResourceEncapsulation/#ClassFilesAsResources
> #ReflectiveAccessToNonExportedTypes
> #AwkwardStrongEncapsulation
> #ReadabilityAddedByLayerCreator
> #IndirectQualifiedReflectiveAccess (partial)
> #VersionsInModuleNames
> #NonHierarchicalLayers
> #ModuleAnnotations/#ModuleDeprecation
> #ReflectiveAccessByInstrumentationAgents
>
> Some of these issues are not "Resolved" yet, meaning there is still ongoing
> discussion on the EG mailing list. That is okay, there is nothing final here.
> If there are changes to these proposals then the implementation changes will
> follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we
> will keep the jake forest open for ongoing prototyping and iteration, also
> ongoing implementation improvements where iteration or bake time is important.
>
> For the code review then the focus is therefore on sanity checking the
> changes that we would like to bring into jdk9/dev. We will not use this
> review thread to debate alternative designs or other big implementation
> changes that are more appropriate to bake in jake.
>
> To get going, I've put the webrevs with a snapshot of the changes in jake
> here:
>http://cr.openjdk.java.net/~alanb/8169069/0/
>
> The changes are currently sync'ed against jdk-9+146 and will be rebased (and
> re-tested) against jdk9/dev prior to integration. There are a number of small
> changes that need to be added to this in the coming days, I will refresh the
> webrev every few days to take account of these updates.
>
>
> A few important points to mention, even if you aren't reviewing the changes:
>
> 1. This refresh requires a new version of jtreg to run the tests. The changes
> for this new version are in the code-tools/jtreg repository and the plan is
> to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then
> we'll update the requiredVersion property in each TEST.ROOT to force everyone
> to update.
>
> 2. For developers trying out modules with the main line JDK 9 builds then be
> aware that `requires public` changes to `requires transitive` and the
> `provides` clause changes to require all providers for a specific service
> type to be in the same clause. Also be aware that the binary form of the
> module declaration (module-info.class) changes so you will need to recompile
> any modules.
>
> 3. Those running existing code on JDK 9 and ignoring modules will need to be
> aware of a disruptive change in this refresh. The disruptive change is
> #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it
> can't be used to break into non-public fields/methods of JDK classes. This
> change is going to expose a lot of hacks in existing code. We plan to send
> mail to jdk9-dev in advance of this integration to create awareness of this
> change. As per the original introduction of strong encapsulation then command
> line options (and now the manifest of application JAR files) can be used to
> keep existing code working. The new option is `--add-opens` to open a package
> in a module for deep reflection by other modules. As an example, if you find
> yourself with code that hacks into the private `comparator` field in
> java.util.TreeMap then running with `--add-opens
> java.base/java.util=ALL-UNNAMED` will keep that code working.
>
>
> A few miscellaneous notes for those that are reviewing:
>
> 1. We have some temporary/transition code in the top-level repo to deal with
> the importing of the JavaFX modules. This will be removed once
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Good to see the exception handling being verbose with context Alan. I've
given a quick look at that area and have only some minor comments.
Layer.java :
635 if (parentLayers.size() != parentConfigurations.size())
636 throw new IllegalArgumentException("wrong number of
parents");
Could we print the number of parents expected versus detected ?
src/jdk.jartool/share/classes/sun/tools/jar/Main.java
856 if (name.startsWith(VERSIONS_DIR)) {
857 throw new InternalError(name);
As unlikely as it is, could we hint at what went wrong ?
.. ("Can't add package beginning with " + VERSIONS_DIR)
some typos :
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java
1076 /**
1077 * Loads a Enum field.
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java
1121 * @return {@code true} if this is a open module
regards,
Sean.
On 24/11/2016 15:25, Alan Bateman wrote:
Folks on jigsaw-dev will know that we are on a mission to bring the
changes accumulated in the jake forest to jdk9/dev. We can think of
this as a refresh of the module system in JDK 9, the last big refresh
was in May with many small updates since then.
The focus this time is to bring the changes that are tied to JSR
issues into jdk9/dev, specifically the issues that are tracked on the
JSR issues list [1] as:
#CompileTimeDependences
#AddExportsInManifest
#ClassFileModuleName
#ClassFileAccPublic
#ServiceLoaderEnhancements
#ResourceEncapsulation/#ClassFilesAsResources
#ReflectiveAccessToNonExportedTypes
#AwkwardStrongEncapsulation
#ReadabilityAddedByLayerCreator
#IndirectQualifiedReflectiveAccess (partial)
#VersionsInModuleNames
#NonHierarchicalLayers
#ModuleAnnotations/#ModuleDeprecation
#ReflectiveAccessByInstrumentationAgents
Some of these issues are not "Resolved" yet, meaning there is still
ongoing discussion on the EG mailing list. That is okay, there is
nothing final here. If there are changes to these proposals then the
implementation changes will follow. Also, as I said in a mail to
jigsaw-dev yesterday [2], is that we will keep the jake forest open
for ongoing prototyping and iteration, also ongoing implementation
improvements where iteration or bake time is important.
For the code review then the focus is therefore on sanity checking the
changes that we would like to bring into jdk9/dev. We will not use
this review thread to debate alternative designs or other big
implementation changes that are more appropriate to bake in jake.
To get going, I've put the webrevs with a snapshot of the changes in
jake here:
http://cr.openjdk.java.net/~alanb/8169069/0/
The changes are currently sync'ed against jdk-9+146 and will be
rebased (and re-tested) against jdk9/dev prior to integration. There
are a number of small changes that need to be added to this in the
coming days, I will refresh the webrev every few days to take account
of these updates.
A few important points to mention, even if you aren't reviewing the
changes:
1. This refresh requires a new version of jtreg to run the tests. The
changes for this new version are in the code-tools/jtreg repository
and the plan is to tag a new build (jtreg4.2-b04) next week. Once the
tag has been added then we'll update the requiredVersion property in
each TEST.ROOT to force everyone to update.
2. For developers trying out modules with the main line JDK 9 builds
then be aware that `requires public` changes to `requires transitive`
and the `provides` clause changes to require all providers for a
specific service type to be in the same clause. Also be aware that the
binary form of the module declaration (module-info.class) changes so
you will need to recompile any modules.
3. Those running existing code on JDK 9 and ignoring modules will need
to be aware of a disruptive change in this refresh. The disruptive
change is #AwkwardStrongEncapsulation where setAccessible(true) is
changed so that it can't be used to break into non-public
fields/methods of JDK classes. This change is going to expose a lot of
hacks in existing code. We plan to send mail to jdk9-dev in advance of
this integration to create awareness of this change. As per the
original introduction of strong encapsulation then command line
options (and now the manifest of application JAR files) can be used to
keep existing code working. The new option is `--add-opens` to open a
package in a module for deep reflection by other modules. As an
example, if you find yourself with code that hacks into the private
`comparator` field in java.util.TreeMap then running with `--add-opens
java.base/java.util=ALL-UNNAMED` will keep that code working.
A few miscellaneous notes for those that are reviewing:
1. We have some temporary/transition code in the top-level repo to
deal with the importing of the JavaFX modules. This will be removed
once the changes are in JDK 9 for t
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Karen, Sorry for delay. I was on vacation last week. I plan to review the changes tomorrow. -Dmitry On 2016-11-28 17:47, Karen Kinnear wrote: > Alan, > > I reviewed all the hotspot runtime changes > - except the tests (Christian will review those) > - and jvmti - which Dmitry Samersoff will review. > > They look good. > > thanks, > Karen > >> On Nov 28, 2016, at 9:32 AM, Lois Foltan wrote: >> >> Hi Alan, >> >> I have reviewed the hotspot changes and they look good. Minor nit, >> src/share/vm/classfile/javaClasses.cpp only differs by the addition of a >> blank line. >> >> Thanks, >> Lois >> >> On 11/24/2016 10:25 AM, Alan Bateman wrote: >>> Folks on jigsaw-dev will know that we are on a mission to bring the changes >>> accumulated in the jake forest to jdk9/dev. We can think of this as a >>> refresh of the module system in JDK 9, the last big refresh was in May with >>> many small updates since then. >>> >>> The focus this time is to bring the changes that are tied to JSR issues >>> into jdk9/dev, specifically the issues that are tracked on the JSR issues >>> list [1] as: >>> >>> #CompileTimeDependences >>> #AddExportsInManifest >>> #ClassFileModuleName >>> #ClassFileAccPublic >>> #ServiceLoaderEnhancements >>> #ResourceEncapsulation/#ClassFilesAsResources >>> #ReflectiveAccessToNonExportedTypes >>> #AwkwardStrongEncapsulation >>> #ReadabilityAddedByLayerCreator >>> #IndirectQualifiedReflectiveAccess (partial) >>> #VersionsInModuleNames >>> #NonHierarchicalLayers >>> #ModuleAnnotations/#ModuleDeprecation >>> #ReflectiveAccessByInstrumentationAgents >>> >>> Some of these issues are not "Resolved" yet, meaning there is still ongoing >>> discussion on the EG mailing list. That is okay, there is nothing final >>> here. If there are changes to these proposals then the implementation >>> changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], >>> is that we will keep the jake forest open for ongoing prototyping and >>> iteration, also ongoing implementation improvements where iteration or bake >>> time is important. >>> >>> For the code review then the focus is therefore on sanity checking the >>> changes that we would like to bring into jdk9/dev. We will not use this >>> review thread to debate alternative designs or other big implementation >>> changes that are more appropriate to bake in jake. >>> >>> To get going, I've put the webrevs with a snapshot of the changes in jake >>> here: >>>http://cr.openjdk.java.net/~alanb/8169069/0/ >>> >>> The changes are currently sync'ed against jdk-9+146 and will be rebased >>> (and re-tested) against jdk9/dev prior to integration. There are a number >>> of small changes that need to be added to this in the coming days, I will >>> refresh the webrev every few days to take account of these updates. >>> >>> >>> A few important points to mention, even if you aren't reviewing the changes: >>> >>> 1. This refresh requires a new version of jtreg to run the tests. The >>> changes for this new version are in the code-tools/jtreg repository and the >>> plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been >>> added then we'll update the requiredVersion property in each TEST.ROOT to >>> force everyone to update. >>> >>> 2. For developers trying out modules with the main line JDK 9 builds then >>> be aware that `requires public` changes to `requires transitive` and the >>> `provides` clause changes to require all providers for a specific service >>> type to be in the same clause. Also be aware that the binary form of the >>> module declaration (module-info.class) changes so you will need to >>> recompile any modules. >>> >>> 3. Those running existing code on JDK 9 and ignoring modules will need to >>> be aware of a disruptive change in this refresh. The disruptive change is >>> #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it >>> can't be used to break into non-public fields/methods of JDK classes. This >>> change is going to expose a lot of hacks in existing code. We plan to send >>> mail to jdk9-dev in advance of this integration to create awareness of this >>> change. As per the original introduction of strong encapsulation then >>> command line options (and now the manifest of application JAR files) can be >>> used to keep existing code working. The new option is `--add-opens` to open >>> a package in a module for deep reflection by other modules. As an example, >>> if you find yourself with code that hacks into the private `comparator` >>> field in java.util.TreeMap then running with `--add-opens >>> java.base/java.util=ALL-UNNAMED` will keep that code working. >>> >>> >>> A few miscellaneous notes for those that are reviewing: >>> >>> 1. We have some temporary/transition code in the top-level repo to deal >>> with the importing of the JavaFX modules. This will be removed once the >>> changes are in JDK 9 for the OpenJFX project to use. >>> >>> 2. In the j
RE: Code review for jigsaw/jake -> jdk9/dev sync up
Hi Alan, I've reviewed the test changes for Hotspot and they look good. Thanks, Christian -Original Message- From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-boun...@openjdk.java.net] On Behalf Of Alan Bateman Sent: Thursday, November 24, 2016 10:25 AM To: jigsaw-dev ; hotspot-runtime-...@openjdk.java.net runtime ; compiler-...@openjdk.java.net Subject: Code review for jigsaw/jake -> jdk9/dev sync up Folks on jigsaw-dev will know that we are on a mission to bring the changes accumulated in the jake forest to jdk9/dev. We can think of this as a refresh of the module system in JDK 9, the last big refresh was in May with many small updates since then. The focus this time is to bring the changes that are tied to JSR issues into jdk9/dev, specifically the issues that are tracked on the JSR issues list [1] as: #CompileTimeDependences #AddExportsInManifest #ClassFileModuleName #ClassFileAccPublic #ServiceLoaderEnhancements #ResourceEncapsulation/#ClassFilesAsResources #ReflectiveAccessToNonExportedTypes #AwkwardStrongEncapsulation #ReadabilityAddedByLayerCreator #IndirectQualifiedReflectiveAccess (partial) #VersionsInModuleNames #NonHierarchicalLayers #ModuleAnnotations/#ModuleDeprecation #ReflectiveAccessByInstrumentationAgents Some of these issues are not "Resolved" yet, meaning there is still ongoing discussion on the EG mailing list. That is okay, there is nothing final here. If there are changes to these proposals then the implementation changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], is that we will keep the jake forest open for ongoing prototyping and iteration, also ongoing implementation improvements where iteration or bake time is important. For the code review then the focus is therefore on sanity checking the changes that we would like to bring into jdk9/dev. We will not use this review thread to debate alternative designs or other big implementation changes that are more appropriate to bake in jake. To get going, I've put the webrevs with a snapshot of the changes in jake here: http://cr.openjdk.java.net/~alanb/8169069/0/ The changes are currently sync'ed against jdk-9+146 and will be rebased (and re-tested) against jdk9/dev prior to integration. There are a number of small changes that need to be added to this in the coming days, I will refresh the webrev every few days to take account of these updates. A few important points to mention, even if you aren't reviewing the changes: 1. This refresh requires a new version of jtreg to run the tests. The changes for this new version are in the code-tools/jtreg repository and the plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been added then we'll update the requiredVersion property in each TEST.ROOT to force everyone to update. 2. For developers trying out modules with the main line JDK 9 builds then be aware that `requires public` changes to `requires transitive` and the `provides` clause changes to require all providers for a specific service type to be in the same clause. Also be aware that the binary form of the module declaration (module-info.class) changes so you will need to recompile any modules. 3. Those running existing code on JDK 9 and ignoring modules will need to be aware of a disruptive change in this refresh. The disruptive change is #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it can't be used to break into non-public fields/methods of JDK classes. This change is going to expose a lot of hacks in existing code. We plan to send mail to jdk9-dev in advance of this integration to create awareness of this change. As per the original introduction of strong encapsulation then command line options (and now the manifest of application JAR files) can be used to keep existing code working. The new option is `--add-opens` to open a package in a module for deep reflection by other modules. As an example, if you find yourself with code that hacks into the private `comparator` field in java.util.TreeMap then running with `--add-opens java.base/java.util=ALL-UNNAMED` will keep that code working. A few miscellaneous notes for those that are reviewing: 1. We have some temporary/transition code in the top-level repo to deal with the importing of the JavaFX modules. This will be removed once the changes are in JDK 9 for the OpenJFX project to use. 2. In the jdk repo then it's important to understand that the module system is initialized at startup and there are many places where we need to keep startup performance in mind. This sometimes means less elegant code than might be used if startup wasn't such a big concern. 3. The changes in the jaxws repo make use of new APIs that means the code doesn't compile with JDK 7 or JDK 8. Our intention is to work with the JAXB and JAX-WS maintainers to address the issues in the upstream project and then bring those changes into jdk9/dev to replace the patche
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Thanks Lois. I removed the blank line. Mandy > On Nov 28, 2016, at 6:32 AM, Lois Foltan wrote: > > Hi Alan, > > I have reviewed the hotspot changes and they look good. Minor nit, > src/share/vm/classfile/javaClasses.cpp only differs by the addition of a > blank line. > > Thanks, > Lois > > On 11/24/2016 10:25 AM, Alan Bateman wrote: >> Folks on jigsaw-dev will know that we are on a mission to bring the changes >> accumulated in the jake forest to jdk9/dev. We can think of this as a >> refresh of the module system in JDK 9, the last big refresh was in May with >> many small updates since then. >> >> The focus this time is to bring the changes that are tied to JSR issues into >> jdk9/dev, specifically the issues that are tracked on the JSR issues list >> [1] as: >> >> #CompileTimeDependences >> #AddExportsInManifest >> #ClassFileModuleName >> #ClassFileAccPublic >> #ServiceLoaderEnhancements >> #ResourceEncapsulation/#ClassFilesAsResources >> #ReflectiveAccessToNonExportedTypes >> #AwkwardStrongEncapsulation >> #ReadabilityAddedByLayerCreator >> #IndirectQualifiedReflectiveAccess (partial) >> #VersionsInModuleNames >> #NonHierarchicalLayers >> #ModuleAnnotations/#ModuleDeprecation >> #ReflectiveAccessByInstrumentationAgents >> >> Some of these issues are not "Resolved" yet, meaning there is still ongoing >> discussion on the EG mailing list. That is okay, there is nothing final >> here. If there are changes to these proposals then the implementation >> changes will follow. Also, as I said in a mail to jigsaw-dev yesterday [2], >> is that we will keep the jake forest open for ongoing prototyping and >> iteration, also ongoing implementation improvements where iteration or bake >> time is important. >> >> For the code review then the focus is therefore on sanity checking the >> changes that we would like to bring into jdk9/dev. We will not use this >> review thread to debate alternative designs or other big implementation >> changes that are more appropriate to bake in jake. >> >> To get going, I've put the webrevs with a snapshot of the changes in jake >> here: >>http://cr.openjdk.java.net/~alanb/8169069/0/ >> >> The changes are currently sync'ed against jdk-9+146 and will be rebased (and >> re-tested) against jdk9/dev prior to integration. There are a number of >> small changes that need to be added to this in the coming days, I will >> refresh the webrev every few days to take account of these updates. >> >> >> A few important points to mention, even if you aren't reviewing the changes: >> >> 1. This refresh requires a new version of jtreg to run the tests. The >> changes for this new version are in the code-tools/jtreg repository and the >> plan is to tag a new build (jtreg4.2-b04) next week. Once the tag has been >> added then we'll update the requiredVersion property in each TEST.ROOT to >> force everyone to update. >> >> 2. For developers trying out modules with the main line JDK 9 builds then be >> aware that `requires public` changes to `requires transitive` and the >> `provides` clause changes to require all providers for a specific service >> type to be in the same clause. Also be aware that the binary form of the >> module declaration (module-info.class) changes so you will need to recompile >> any modules. >> >> 3. Those running existing code on JDK 9 and ignoring modules will need to be >> aware of a disruptive change in this refresh. The disruptive change is >> #AwkwardStrongEncapsulation where setAccessible(true) is changed so that it >> can't be used to break into non-public fields/methods of JDK classes. This >> change is going to expose a lot of hacks in existing code. We plan to send >> mail to jdk9-dev in advance of this integration to create awareness of this >> change. As per the original introduction of strong encapsulation then >> command line options (and now the manifest of application JAR files) can be >> used to keep existing code working. The new option is `--add-opens` to open >> a package in a module for deep reflection by other modules. As an example, >> if you find yourself with code that hacks into the private `comparator` >> field in java.util.TreeMap then running with `--add-opens >> java.base/java.util=ALL-UNNAMED` will keep that code working. >> >> >> A few miscellaneous notes for those that are reviewing: >> >> 1. We have some temporary/transition code in the top-level repo to deal with >> the importing of the JavaFX modules. This will be removed once the changes >> are in JDK 9 for the OpenJFX project to use. >> >> 2. In the jdk repo then it's important to understand that the module system >> is initialized at startup and there are many places where we need to keep >> startup performance in mind. This sometimes means less elegant code than >> might be used if startup wasn't such a big concern. >> >> 3. The changes in the jaxws repo make use of new APIs that means the code >> doesn't compile with
Re: Code review for jigsaw/jake -> jdk9/dev sync up
On 28/11/2016 14:47, Chris Hegarty wrote: : 2) jartool Main.java Maybe concealedPackages should have a comment about its use ( it is used in the Validator, and not by Main at all ). Just on this one, I think this was introduced when Steve added the MR JAR validation, I agree it's ugly but we haven't changed it here. I'll add a comment as you suggest but it does some overall cleanup as part of finishing up the MR JAR work. -Alan
Re: Code review for jigsaw/jake -> jdk9/dev sync up
Thanks to everyone that have been reviewing these changes. I've pushed an updated set of webrevs here: http://cr.openjdk.java.net/~alanb/8169069/1/ The update includes changes to address most of the reviewer comments so far. There are a few more cleanups that still need to be pushed today but I think we are otherwise in reasonable shape at this point. As I said in the original mail, we will keep the jake forest open as there are still issues to work on after this integration. -Alan
Re: Code review for jigsaw/jake -> jdk9/dev sync up
test/java/security/testlibrary/Proc.java:
if (hasModules) {
Stream.of(jdk.internal.misc.VM.getRuntimeArguments())
-.filter(arg -> arg.startsWith("--add-exports="))
+.filter(arg -> arg.startsWith("--add-exports=") ||
+ arg.startsWith("--add-opens"))
.forEach(cmd::add);
}
Maybe "--add-opens=" to be more precise?
Thanks
Max
