Initial webrev with changes for JDK 9

[Alan Bateman]

I've pushed webrevs with the initial changes for JDK 9 here:
http://cr.openjdk.java.net/~alanb/8142968/0/

This is a snapshot of what is currently in the jigsaw/jake forest. Our 
mission over the next few weeks is to iterate on this and get it to the 
point where we + Reviewers are happy that it is a reasonable/acceptable 
state to bring into JDK 9. We will need to set a deadline so that we can 
plan the integration, more on this soon. As per our previous milestones 
(JEP 201 and JEP 220) then we'll ask that the integration from jdk9/dev 
to master skip a beat so that the module system is the only change in 
master that week.


It's important to remember that the initial push to JDK 9 is exactly 
that, it's not the final bits. There are many areas that still need 
work, there are many open issues, there is an ongoing JSR, and of course 
there will be ongoing feedback that will help us get it right.


Another important thing to say is this isn't a module system design or 
API review. Questions/comments on the module system design can be 
brought up here of course but we might have to punt to 
jpms-spec-comments on topics that involve JSR discussions. For the API 
then I expect it will go through many iterations, as every good API does.



The following is a summary of what is in each repository, this might 
help to get a feel for where the implementation is currently at.



** top-level repository **

Most of the build changes have already been pushed to JDK 9 as part of 
JEP 201 and subsequent iteration. There are some additional 
jake-specific build changes, most of it is in the top-level repository 
with some changes in other repositories too.


Of significance is that the temporary modules.xml document in the root 
directory is gone, this has been replaced by a module declaration in the 
source code of each module. The other significant thing is that the 
build creates a packaged module for each standard and JDK module. It 
also uses the jlink tool to create the JDK and JRE run-time images.


Erik Joelsson will take point for all the build changes, with help from 
Reviewers from the build group.



** hotspot repository **

At a high-level, the changes in hotspot repository adds support for 
modules to the virtual machine with access control extended to modules.


Startup has been significantly reworked into a sequence of phases, akin 
to runlevels, so that the module system is initialized before any code 
outside of the base module is loaded.


What we used to know as the boot class path is mostly gone except for 
agent and -Xbootclasspath/a cases. Classes are instead loaded from 
modules defined to boot loader. There is also support for patching 
modules for testing and ad hoc needs.


JNI has new functions, as has JVM TI with initial support for debuggers 
and agents that instrument code in modules. There is additional work on 
JVM TI in progress so expect to see more in later webrevs.


There are a number of diagnosability improvements, include improved 
exception messages and support for module details in stack traces.


There are many new tests. There are also updates to many existing tests. 
Christian Tornqvist is planning to bring at least some of test changes 
into jdk9/dev so we should see the patch reduce a bit once we sync up.


Lois Foltan will take point on the hotspot repository, she has lined up 
several Reviewers in the hotspot group to help.



** langtools repository **

As expected, the javac compiler is significantly updated to support 
compilation containing module declarations. The javadoc tool and doclet 
code has also involve significant changes.


When compiling then there are several new command-line options, support 
for module paths, and new compilation modes. JEP 261 has useful 
descriptions.


The jdeps tool has been upgraded with many new options. The javap tool 
has also been updated.


This repository also has the initial updates to the javax.tools and 
javax.lang.model APIs.


There are a lot of updates to existing tests in the webrev and many new 
tests too.


Jonathan Gibbons will take point for the langtools repository and I 
expect will use Reviewers from the compiler group to help get through this.



** jdk repository **

There are a lot of changes in this repository.

One of the most obvious is that there is a module-info.java source file 
in each module's directory.


There is a new java.lang.module API to support module descriptors and to 
create configurations of modules. There are new APIs in 
java.lang.reflect to represent modules and layers of modules.


There are is a lot of support code for the module system itself, for 
example ModuleBootstrap is the class that creates the configuration and 
creates the boot layer.


The application and extension class loaders have been replaced with a 
new implementation based on BuiltinClassLoader that supports loading 
classes/resources from modules (in addition to the class path). Note 
that there are no chang

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 3, 2016, at 6:38 AM, Alan Bateman  wrote:
> 
> I've pushed webrevs with the initial changes for JDK 9 here:
>http://cr.openjdk.java.net/~alanb/8142968/0/

I have reviewed changes for java.management and java.logging module.

src/java.management/share/classes/sun/management/ThreadInfoCompositeData.java
 197 private static final String[] threadInfoV9Attributes = {
 198 DAEMON,
 199 PRIORITY,
 200 MODULE_NAME,
 201 MODULE_VERSION,

Are line 200-201 left over from some earlier prototype?  I think they should be 
removed.

src/java.management/share/classes/sun/management/TypeVersionMapper.java
  Formatting nit: throws in several method signature better to be indented.

I also reviewed the removal of the service config files in the following 
modules:
jdk.attach
jdk.jvmstat
jdk.jvmstat.rmi

Mandy

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

I've refreshed the webrevs here:
  http://cr.openjdk.java.net/~alanb/8142968/1

so that we have a snapshot of what is currently in the jigsaw/jake 
forest. The webrves are against jdk-9+108.


I plan to send mail to jdk9-dev soon proposing that we integrate a 
snapshot into JDK 9 before the end of March.


-Alan.

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

On 08/03/2016 00:15, Mandy Chung wrote:

:

src/java.management/share/classes/sun/management/ThreadInfoCompositeData.java
  197 private static final String[] threadInfoV9Attributes = {
  198 DAEMON,
  199 PRIORITY,
  200 MODULE_NAME,
  201 MODULE_VERSION,

Are line 200-201 left over from some earlier prototype?  I think they should be 
removed.


I think you are right, these should not be needed. There is a 
compatibility test for this and I assume it will continue to pass if 
these are removed.


-Alan

Re: Initial webrev with changes for JDK 9

[Naoto Sato]

Hello,

I reviewed ResourceBundle code and related locale data changes. Overall 
it looks good to me. Here are some minor comments:


java.util.ResourceBundle.java

- In the class description, there are two occurrences of example 
explaing service provider type (i.e., basename+"Provider"). It seems a 
bit redundant. If they should be there in both locations, then I'd use 
the same example. Currently, one base name is "p.MyResources", and the 
other is "com.example.app.MyResources".


- This is sort of a hypothetical situation but what if a named module 
provides both local ResourceBundle, and a ResourceBundleProvider that 
"happens" to have that base name but returns a different bundle 
implementation? I guess ResourceBundleProvider wins, and I would expect 
that precedence described somewhere.


- Line 626-630: This comment of CacheKey class should include some 
description for the newly added "module" key. Same is true for line 
1620-1623.


sun.util.resources.LocaleData.java

- line 233: Can be removed, as it is redundant.

Naoto

On 3/8/16 7:48 AM, Alan Bateman wrote:


I've refreshed the webrevs here:
   http://cr.openjdk.java.net/~alanb/8142968/1

so that we have a snapshot of what is currently in the jigsaw/jake
forest. The webrves are against jdk-9+108.

I plan to send mail to jdk9-dev soon proposing that we integrate a
snapshot into JDK 9 before the end of March.

-Alan.

Re: Initial webrev with changes for JDK 9

[Seán Coffey]

I'm hoping we can bulk up some of the new exception handling. I've put 
suggested edits for the jdk repo together in a webrev. No major edits 
here but it should help supportability of the code for the future. Will 
it be possible to import these in before the bulk push ?


http://cr.openjdk.java.net/~coffeys/webrev.jake_edits/webrev/

Regards,
Sean.

On 03/03/2016 14:38, Alan Bateman wrote:


I've pushed webrevs with the initial changes for JDK 9 here:
http://cr.openjdk.java.net/~alanb/8142968/0/

This is a snapshot of what is currently in the jigsaw/jake forest. Our 
mission over the next few weeks is to iterate on this and get it to 
the point where we + Reviewers are happy that it is a 
reasonable/acceptable state to bring into JDK 9. We will need to set a 
deadline so that we can plan the integration, more on this soon. As 
per our previous milestones (JEP 201 and JEP 220) then we'll ask that 
the integration from jdk9/dev to master skip a beat so that the module 
system is the only change in master that week.


It's important to remember that the initial push to JDK 9 is exactly 
that, it's not the final bits. There are many areas that still need 
work, there are many open issues, there is an ongoing JSR, and of 
course there will be ongoing feedback that will help us get it right.


Another important thing to say is this isn't a module system design or 
API review. Questions/comments on the module system design can be 
brought up here of course but we might have to punt to 
jpms-spec-comments on topics that involve JSR discussions. For the API 
then I expect it will go through many iterations, as every good API does.



The following is a summary of what is in each repository, this might 
help to get a feel for where the implementation is currently at.



** top-level repository **

Most of the build changes have already been pushed to JDK 9 as part of 
JEP 201 and subsequent iteration. There are some additional 
jake-specific build changes, most of it is in the top-level repository 
with some changes in other repositories too.


Of significance is that the temporary modules.xml document in the root 
directory is gone, this has been replaced by a module declaration in 
the source code of each module. The other significant thing is that 
the build creates a packaged module for each standard and JDK module. 
It also uses the jlink tool to create the JDK and JRE run-time images.


Erik Joelsson will take point for all the build changes, with help 
from Reviewers from the build group.



** hotspot repository **

At a high-level, the changes in hotspot repository adds support for 
modules to the virtual machine with access control extended to modules.


Startup has been significantly reworked into a sequence of phases, 
akin to runlevels, so that the module system is initialized before any 
code outside of the base module is loaded.


What we used to know as the boot class path is mostly gone except for 
agent and -Xbootclasspath/a cases. Classes are instead loaded from 
modules defined to boot loader. There is also support for patching 
modules for testing and ad hoc needs.


JNI has new functions, as has JVM TI with initial support for 
debuggers and agents that instrument code in modules. There is 
additional work on JVM TI in progress so expect to see more in later 
webrevs.


There are a number of diagnosability improvements, include improved 
exception messages and support for module details in stack traces.


There are many new tests. There are also updates to many existing 
tests. Christian Tornqvist is planning to bring at least some of test 
changes into jdk9/dev so we should see the patch reduce a bit once we 
sync up.


Lois Foltan will take point on the hotspot repository, she has lined 
up several Reviewers in the hotspot group to help.



** langtools repository **

As expected, the javac compiler is significantly updated to support 
compilation containing module declarations. The javadoc tool and 
doclet code has also involve significant changes.


When compiling then there are several new command-line options, 
support for module paths, and new compilation modes. JEP 261 has 
useful descriptions.


The jdeps tool has been upgraded with many new options. The javap tool 
has also been updated.


This repository also has the initial updates to the javax.tools and 
javax.lang.model APIs.


There are a lot of updates to existing tests in the webrev and many 
new tests too.


Jonathan Gibbons will take point for the langtools repository and I 
expect will use Reviewers from the compiler group to help get through 
this.



** jdk repository **

There are a lot of changes in this repository.

One of the most obvious is that there is a module-info.java source 
file in each module's directory.


There is a new java.lang.module API to support module descriptors and 
to create configurations of modules. There are new APIs in 
java.lang.reflect to represent modules and layers of modules.


There

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 8, 2016, at 1:37 PM, Naoto Sato  wrote:
> 
> Hello,
> 
> I reviewed ResourceBundle code and related locale data changes. Overall it 
> looks good to me. Here are some minor comments:
> 
> java.util.ResourceBundle.java
> 
> - In the class description, there are two occurrences of example explaing 
> service provider type (i.e., basename+"Provider"). It seems a bit redundant. 
> If they should be there in both locations, then I'd use the same example. 
> Currently, one base name is "p.MyResources", and the other is 
> "com.example.app.MyResources”.

I changed it to com.example.app.

> - This is sort of a hypothetical situation but what if a named module 
> provides both local ResourceBundle, and a ResourceBundleProvider that 
> "happens" to have that base name but returns a different bundle 
> implementation? I guess ResourceBundleProvider wins,

If the module provides ResourceBundleProvider, it will not search its local 
resource bundle.

> and I would expect that precedence described somewhere.
> 

It’s described here:

 246  * In named modules, the loaded service providers for the given base name 
are
 247  * used to load resource bundles. If no service providers are available, 
or if
 248  * none of the service providers returns a resource bundle and the caller 
module
 249  * doesn't have its own service provider, the {@code getBundle} factory 
method
 250  * searches for resource bundles local to the caller module.

> - Line 626-630: This comment of CacheKey class should include some 
> description for the newly added "module" key. Same is true for line 1620-1623.
> 
> sun.util.resources.LocaleData.java
> 
> - line 233: Can be removed, as it is redundant.
> 

Removed.

Mandy

> Naoto
> 
> On 3/8/16 7:48 AM, Alan Bateman wrote:
>> 
>> I've refreshed the webrevs here:
>>   http://cr.openjdk.java.net/~alanb/8142968/1
>> 
>> so that we have a snapshot of what is currently in the jigsaw/jake
>> forest. The webrves are against jdk-9+108.
>> 
>> I plan to send mail to jdk9-dev soon proposing that we integrate a
>> snapshot into JDK 9 before the end of March.
>> 
>> -Alan.

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

On 08/03/2016 22:06, Seán Coffey wrote:
I'm hoping we can bulk up some of the new exception handling. I've put 
suggested edits for the jdk repo together in a webrev. No major edits 
here but it should help supportability of the code for the future. 
Will it be possible to import these in before the bulk push ?


http://cr.openjdk.java.net/~coffeys/webrev.jake_edits/webrev/
Thanks for bringing this up. No issue improving the exception messages, 
esp. incorrect API mis-use. I note that you've changed several 
InternalError where the JDK must be completely hosed (`java -version` 
would fail) if they were to arise.


Given that there will be significant code churn for several months then 
maybe it would be better to do a pass over the exceptions closer to JDK 
9 FC. Would you be okay with that? We can create an issue in JIRA to 
make sure that it doesn't get forgotten.


-Alan

Re: Initial webrev with changes for JDK 9

[Seán Coffey]

On 09/03/2016 08:58, Alan Bateman wrote:


On 08/03/2016 22:06, Seán Coffey wrote:
I'm hoping we can bulk up some of the new exception handling. I've 
put suggested edits for the jdk repo together in a webrev. No major 
edits here but it should help supportability of the code for the 
future. Will it be possible to import these in before the bulk push ?


http://cr.openjdk.java.net/~coffeys/webrev.jake_edits/webrev/
Thanks for bringing this up. No issue improving the exception 
messages, esp. incorrect API mis-use. I note that you've changed 
several InternalError where the JDK must be completely hosed (`java 
-version` would fail) if they were to arise.
Agreed - important for end user to know what the JDK's last breath was 
all about.


Given that there will be significant code churn for several months 
then maybe it would be better to do a pass over the exceptions closer 
to JDK 9 FC. Would you be okay with that? We can create an issue in 
JIRA to make sure that it doesn't get forgotten.
Yes - that works. I'll put forward a patch when this is in jdk9/dev. I 
was hoping to would be easier to target corrections before it entered 
the mainline.


regards,
Sean.


-Alan

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

I've refreshed the webrevs here:
   http://cr.openjdk.java.net/~alanb/8142968/2

so that we have a snapshot of what is currently in the jigsaw/jake 
forest. The webrevs are against jdk-9+109.


As I said in the last mail, we would like to integrate this snapshot 
into JDK 9 before the end of March. The proposal is to aim to integrate 
during the week of March 21, with the the week of March 28 as fallback 
in event of problems.


The related JEPs (JEP 200, JEP 260, JEP 261 and JEP 282) are now in 
"Proposed to Target" state. Mark send mail to jdk9-dev with this list 
and other proposals yesterday.


-Alan

Re: Initial webrev with changes for JDK 9

[Paul Sandoz]

> On 11 Mar 2016, at 10:39, Alan Bateman  wrote:
> 
> 
> I've refreshed the webrevs here:
>   http://cr.openjdk.java.net/~alanb/8142968/2
> 

I browsed the code. Generally the feeling i get is it’s of good quality. 
Previously some of this area was quite legacy and it is refreshing to view more 
modern code.

Comments below, only the first is something i think we should really change, 
the others are up to you.

Once Jigsaw goes in i wonder if we should opportunistically revisit the use of 
URL and Enumeration with class loaders?

Paul.


sun/util/locale/provider/ResourceBundleProviderSupport.java
—


  72 // java.base may not be able to read the bundleClass's 
module.
  73 PrivilegedAction pa1 = () -> { 
ctor.setAccessible(true); return null; };
  74 AccessController.doPrivileged(pa1);
  75 try {
  76 return ctor.newInstance((Object[]) null);
  77 } catch (InvocationTargetException e) {
  78 
Unsafe.getUnsafe().throwException(e.getTargetException());
  79 } catch (InstantiationException | IllegalAccessException 
e) {
  80 throw new InternalError(e);
  81 }
  82 } catch (NoSuchMethodException e) {
  83 }
  84 }
  85 return null;
  86 }


Please avoid the use of Unsafe here, use a sneaky throw instead if necessary, 
or wrap.

That Unsafe method needs to die!


j.l.Class
—


2476 @CallerSensitive
2477 public URL getResource(String name) {
2478 name = resolveName(name);
2479
2480 // if this Class and the caller are in the same named module
2481 // then attempt to get URL to the resource in the module
2482 Module module = getModule();
2483 if (module.isNamed()) {
2484 Class caller = Reflection.getCallerClass();
2485 if (caller != null && caller.getModule() == module) {
2486 String mn = getModule().getName();
2487 ClassLoader cl = getClassLoader0();
2488 try {
2489 if (cl == null) {
2490 return BootLoader.findResource(mn, name);
2491 } else {
2492 return cl.findResource(mn, name);
2493 }
2494 } catch (IOException ioe) {
2495 return null;
2496 }
2497 }
2498 }


The method getResourceAsStream has an optimisation for a BuiltinClassLoader:

2412 // special-case built-in class loaders to avoid the
2413 // need for a URL connection
2414 if (cl == null) {
2415 return BootLoader.findResourceAsStream(mn, name);
2416 } else if (cl instanceof BuiltinClassLoader) {
2417 return ((BuiltinClassLoader) 
cl).findResourceAsStream(mn, name);
2418 } else {
2419 URL url = cl.findResource(mn, name);
2420 return (url != null) ? url.openStream() : null;
2421 }

Is that something you plan to include for getResource too at some point?



j.l.ClassLoader
—

1777 // define Package object if the named package is not yet defined
1778 NamedPackage p = packages.computeIfAbsent(name,
1779   k -> 
NamedPackage.toPackage(name, m));
1780 if (p instanceof Package)
1781 return (Package)p;
1782
1783 // otherwise, replace the NamedPackage object with Package object
1784 return (Package)packages.compute(name, this::toPackage);

You could merge the computeIfAbsent and compute into a single compute call if 
you so wish.

return (Package)packages.compute((n, p) -> {
  // define Package object if the named package is not yet defined
  if (p == null) return NamedPackage.toPackage(name, m);
  // otherwise, replace the NamedPackage object with Package object
  return (p instanceof Package) ? p : toPackage(n, p);
});



2553 /**
2554  * Returns the ServiceCatalog for modules defined to this class loader,
2555  * creating it if it doesn't already exist.
2556  */
2557 ServicesCatalog createOrGetServicesCatalog() {
2558 ServicesCatalog catalog = servicesCatalog;
2559 if (catalog == null) {
2560 catalog = new ServicesCatalog();
2561 boolean set = trySetObjectField("servicesCatalog", catalog);
2562 if (!set) {
2563 // beaten by someone else
2564 catalog = servicesCatalog;
2565 }
2566 }
2567 return catalog;
2568 }
2569
2570 // the ServiceCatalog for modules associated with this class loader.
2571 private volatile ServicesCatalog servicesCatalog;
2572
2573
2574 /**
2575  * Attempts to atomically set 

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

On 11/03/2016 16:50, Paul Sandoz wrote:

:
I browsed the code. Generally the feeling i get is it’s of good quality. 
Previously some of this area was quite legacy and it is refreshing to view more 
modern code.
Thanks for going through it. There is quite a mix here, lots of new code 
but lots of ancient code has to be changed along the way.




Comments below, only the first is something i think we should really change, 
the others are up to you.

Once Jigsaw goes in i wonder if we should opportunistically revisit the use of 
URL and Enumeration with class loaders?
Potentially, it's just hasn't been interesting so far because the legacy 
ClassLoader methods aren't specified to locate resources in modules.




:


The method getResourceAsStream has an optimisation for a BuiltinClassLoader:

:

Is that something you plan to include for getResource too at some point?
getResource returns a URL so the optimization to avoid the going through 
the URL protocol handler isn't applicable.





If you can syncrhonize on this, just use double checked locking?
I'll look at it, part of the reason for the currently implementation is 
that we were setting several fields.







j.l.i.BoundMethodHandle
—

  791 // ## FIXME
  792 // assert 
F_SPECIES_DATA.getDeclaredAnnotation(Stable.class) != null;

That’s odd. Why did that need commenting out?
getDeclarationAnnotation => annotation parsing and creating proxies and 
too early in the VM startup for that. It was commented out in one of the 
numerous sync ups I think.


I'll ask a comment to it.





MethodHandles
—

That’s quite some trick to create PUBLIC_LOOKUP :-) spinning up a new class 
(“Unamed”) in a new class loader. If that mechanism is gonna stay and If there 
are costs associated with that, we could easily skip the ASM part and use hard 
coded bytes.
I think TBD, I think John would like PL to revert to using Object as the 
lookup class but requires extending the lookup modes. I have an issue in 
JIRA to track another phase of work here.




:




  198 while (c.isArray()) {
  199 c = c.getComponentType();
  200 }

This pattern is occurring a number of times.

Memo: add a method on Arrays, or somewhere…,
I agree, we have several places that need the package name but they can 
be called with array objects.






sun.net.www.protocol.jrt.JavaRuntimeURLConnection
—

   56 private static final ImageReader reader = 
ImageReaderFactory.getImageReader();

reader -> READER

okay.




java.lang.module.ModuleInfo
—

:

Consider using Set.of

I agree, it pre-dates Set.of of course.




j.l.ModuleReader
—

:

Use InputStream.readAllBytes?

I meant to do that this, this code pre-dates readAllBytes.





jdk.internal.module.Builder
—

   56 private static final Set MANDATED =
   57 Collections.singleton(Requires.Modifier.MANDATED);
   58 private static final Set PUBLIC =
   59 Collections.singleton(Requires.Modifier.PUBLIC);

Set.of ?

Okay.

Re: Initial webrev with changes for JDK 9

[Sean Mullan]

Myself (mullan), Tony (ascarpino), and Vinnie (vinnie) reviewed the 
security-libs and java.naming changes and did not find any issues. So 
it's good to go from our perspective.


--Sean

On 03/11/2016 04:39 AM, Alan Bateman wrote:


I've refreshed the webrevs here:
http://cr.openjdk.java.net/~alanb/8142968/2

so that we have a snapshot of what is currently in the jigsaw/jake
forest. The webrevs are against jdk-9+109.

As I said in the last mail, we would like to integrate this snapshot
into JDK 9 before the end of March. The proposal is to aim to integrate
during the week of March 21, with the the week of March 28 as fallback
in event of problems.

The related JEPs (JEP 200, JEP 260, JEP 261 and JEP 282) are now in
"Proposed to Target" state. Mark send mail to jdk9-dev with this list
and other proposals yesterday.

-Alan

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

Thanks for the feedback.

> On Mar 11, 2016, at 8:50 AM, Paul Sandoz  wrote:
> :
> 
> sun/util/locale/provider/ResourceBundleProviderSupport.java
> —
> 
> 
>  72 // java.base may not be able to read the bundleClass's 
> module.
>  73 PrivilegedAction pa1 = () -> { 
> ctor.setAccessible(true); return null; };
>  74 AccessController.doPrivileged(pa1);
>  75 try {
>  76 return ctor.newInstance((Object[]) null);
>  77 } catch (InvocationTargetException e) {
>  78 
> Unsafe.getUnsafe().throwException(e.getTargetException());
>  79 } catch (InstantiationException | IllegalAccessException 
> e) {
>  80 throw new InternalError(e);
>  81 }
>  82 } catch (NoSuchMethodException e) {
>  83 }
>  84 }
>  85 return null;
>  86 }
> 
> 
> Please avoid the use of Unsafe here, use a sneaky throw instead if necessary, 
> or wrap.

I agree. It's also used from ResourceBundle.Control::newBundle. Since core 
reflection now gets readability for free, setAccessible is no longer needed 
[1]. So it can call Class::newInstance instead.  I fixed both places not to use 
Unsafe.

> 
> j.l.ClassLoader
> —
> 
> 1777 // define Package object if the named package is not yet defined
> 1778 NamedPackage p = packages.computeIfAbsent(name,
> 1779   k -> 
> NamedPackage.toPackage(name, m));
> 1780 if (p instanceof Package)
> 1781 return (Package)p;
> 1782
> 1783 // otherwise, replace the NamedPackage object with Package object
> 1784 return (Package)packages.compute(name, this::toPackage);
> 
> You could merge the computeIfAbsent and compute into a single compute call if 
> you so wish.
> 
> return (Package)packages.compute((n, p) -> {
>  // define Package object if the named package is not yet defined
>  if (p == null) return NamedPackage.toPackage(name, m);
>  // otherwise, replace the NamedPackage object with Package object
>  return (p instanceof Package) ? p : toPackage(n, p);
> });
> 

Thanks for the example.  I will keep it as is.

> 
> j.l.r.Proxy
> —
> 
> 636 private static final WeakCache>, Class> 
> proxyCache =
> 637 new WeakCache<>(new KeyFactory(),
> 638 new BiFunction>, Class>()  {
> 639 @Override
> 640 public Class apply(Module m, List> 
> interfaces) {
> 641 Objects.requireNonNull(m);
> 642 return defineProxyClass(m, interfaces);
> 643 }
> 644 });
> 
> Wild idea: this feels like an equivalent of j.l.i.ClassValue for Module.
> 

Feels like that.

This is the case when the proxy class is generated in a dynamic module case 
[1]. It’s a computed value  with the given list of proxy interfaces and the 
given defining class loader.

Each dynamic proxy is defined by a given class loader. The target Module where 
the proxy class may be a newly defined dynamic module per the given class 
loader.  So a given list of proxy interfaces + the defining class loader will 
map to one Module.

> 
> :
> Use Stream.flatMap e.g.
> 
>  interfaces.stream().flatMap(i -> Stream.of(i.getMethods()).
>flatMap(m -> f(m) /*extract all types on method*/).
>map(…).filter(…).collect(toSet())
> 
> Stream> f(Method m) {
>  return Stream.of(new Class[] {m.getReturnType()}, m.getParameterTypes(), 
> m.getExceptionTypes()).
>flatMap(a -> Stream.of(a));
> }

Updated.

> 
> Consider using computeIfAbsent. At the moment no need for AtomicInteger.
> 
> 

Updated.  I probably intended to convert that when converting to AtomicInteger.

> 1055 @CallerSensitive
> 1056 public static Object newProxyInstance(ClassLoader loader,
> 1057   Class[] interfaces,
> 1058   InvocationHandler h) {
> 1059 Objects.requireNonNull(h);
> 1060
> 1061 final List> intfs = Arrays.asList(interfaces.clone());
> 
> Use List.of, which will also clone? This presumes that there are no null 
> values in the array.

Updated.  Happy to use the new JDK 9 List.of method (the code predates 
List.of). Yes it will clone and do null check. 
> 
> sun.invoke.util.VerifyAccess
> —
> 
> 198 while (c.isArray()) {
> 199 c = c.getComponentType();
> 200 }
> 
> This pattern is occurring a number of times.
> 
> Memo: add a method on Arrays, or somewhere…,
> 

+1.  Perhaps Class::getBaseElementType.

> 
> sun.util.resources.LocaleData
> —
> 
>  77 private static final Map> candidatesMap = new 
> ConcurrentHashMap<>();
> 
> candidatesMap -> CANDIDATES_MAP
> 

Changed.

Alan has covered the rest.

Mandy

[1] 
http://mail.openjdk.java.net/pipermail/jpms-spec-experts/2016-March/000248.html
[2] 

Re: Initial webrev with changes for JDK 9

[Paul Sandoz]

> On 11 Mar 2016, at 18:53, Alan Bateman  wrote:
> 
> On 11/03/2016 16:50, Paul Sandoz wrote:
>> :
>> I browsed the code. Generally the feeling i get is it’s of good quality. 
>> Previously some of this area was quite legacy and it is refreshing to view 
>> more modern code.
> Thanks for going through it. There is quite a mix here, lots of new code but 
> lots of ancient code has to be changed along the way.
> 
>> 
>> Comments below, only the first is something i think we should really change, 
>> the others are up to you.
>> 
>> Once Jigsaw goes in i wonder if we should opportunistically revisit the use 
>> of URL and Enumeration with class loaders?
> Potentially, it's just hasn't been interesting so far because the legacy 
> ClassLoader methods aren't specified to locate resources in modules.
> 

Understand. Just looking for opportunities to "rm Enumeration" and potentially 
improve how URIs are processed by encouraging the use of “java.net.URI” instead 
of “java.net.URL” (even if one has to transform the former to the latter to 
open a connection).


> 
>> :
>> 
>> 
>> The method getResourceAsStream has an optimisation for a BuiltinClassLoader:
>> 
>> :
>> 
>> Is that something you plan to include for getResource too at some point?
> getResource returns a URL so the optimization to avoid the going through the 
> URL protocol handler isn't applicable.
> 

Ok.


> 
>> 
>> If you can syncrhonize on this, just use double checked locking?
> I'll look at it, part of the reason for the currently implementation is that 
> we were setting several fields.
> 

AFAICT it only CAS’es one field.


> 
>> 
>> 
>> 
>> j.l.i.BoundMethodHandle
>> —
>> 
>>  791 // ## FIXME
>>  792 // assert 
>> F_SPECIES_DATA.getDeclaredAnnotation(Stable.class) != null;
>> 
>> That’s odd. Why did that need commenting out?
> getDeclarationAnnotation => annotation parsing and creating proxies and too 
> early in the VM startup for that. It was commented out in one of the numerous 
> sync ups I think.
> 

Ah!


> I'll ask a comment to it.
> 
> 
>> 
>> 
>> MethodHandles
>> —
>> 
>> That’s quite some trick to create PUBLIC_LOOKUP :-) spinning up a new class 
>> (“Unamed”) in a new class loader. If that mechanism is gonna stay and If 
>> there are costs associated with that, we could easily skip the ASM part and 
>> use hard coded bytes.
> I think TBD, I think John would like PL to revert to using Object as the 
> lookup class but requires extending the lookup modes. I have an issue in JIRA 
> to track another phase of work here.
> 

Ok. This is a gnarly area.

Paul.

Re: Initial webrev with changes for JDK 9

[Paul Sandoz]

> On 11 Mar 2016, at 21:53, Mandy Chung  wrote:
> 
> Thanks for the feedback.
> 
>> On Mar 11, 2016, at 8:50 AM, Paul Sandoz  wrote:
>> :
>> 
>> sun/util/locale/provider/ResourceBundleProviderSupport.java
>> —
>> 
>> 
>> 72 // java.base may not be able to read the bundleClass's 
>> module.
>> 73 PrivilegedAction pa1 = () -> { 
>> ctor.setAccessible(true); return null; };
>> 74 AccessController.doPrivileged(pa1);
>> 75 try {
>> 76 return ctor.newInstance((Object[]) null);
>> 77 } catch (InvocationTargetException e) {
>> 78 
>> Unsafe.getUnsafe().throwException(e.getTargetException());
>> 79 } catch (InstantiationException | IllegalAccessException 
>> e) {
>> 80 throw new InternalError(e);
>> 81 }
>> 82 } catch (NoSuchMethodException e) {
>> 83 }
>> 84 }
>> 85 return null;
>> 86 }
>> 
>> 
>> Please avoid the use of Unsafe here, use a sneaky throw instead if 
>> necessary, or wrap.
> 
> I agree. It's also used from ResourceBundle.Control::newBundle. Since core 
> reflection now gets readability for free, setAccessible is no longer needed 
> [1]. So it can call Class::newInstance instead.  I fixed both places not to 
> use Unsafe.
> 

Thanks!


>> 
>> j.l.ClassLoader
>> —
>> 
>> 1777 // define Package object if the named package is not yet defined
>> 1778 NamedPackage p = packages.computeIfAbsent(name,
>> 1779   k -> 
>> NamedPackage.toPackage(name, m));
>> 1780 if (p instanceof Package)
>> 1781 return (Package)p;
>> 1782
>> 1783 // otherwise, replace the NamedPackage object with Package 
>> object
>> 1784 return (Package)packages.compute(name, this::toPackage);
>> 
>> You could merge the computeIfAbsent and compute into a single compute call 
>> if you so wish.
>> 
>> return (Package)packages.compute((n, p) -> {
>> // define Package object if the named package is not yet defined
>> if (p == null) return NamedPackage.toPackage(name, m);
>> // otherwise, replace the NamedPackage object with Package object
>> return (p instanceof Package) ? p : toPackage(n, p);
>> });
>> 
> 
> Thanks for the example.  I will keep it as is.

Ok. One advantage of the merged-approach is it’s atomic. The former may update 
an entry twice if there is a race, but that may not matter.


> 
>> 
>> j.l.r.Proxy
>> —
>> 
>> 636 private static final WeakCache>, Class> 
>> proxyCache =
>> 637 new WeakCache<>(new KeyFactory(),
>> 638 new BiFunction>, Class>()  {
>> 639 @Override
>> 640 public Class apply(Module m, List> 
>> interfaces) {
>> 641 Objects.requireNonNull(m);
>> 642 return defineProxyClass(m, interfaces);
>> 643 }
>> 644 });
>> 
>> Wild idea: this feels like an equivalent of j.l.i.ClassValue for Module.
>> 
> 
> Feels like that.
> 
> This is the case when the proxy class is generated in a dynamic module case 
> [1]. It’s a computed value  with the given list of proxy interfaces and the 
> given defining class loader.
> 
> Each dynamic proxy is defined by a given class loader. The target Module 
> where the proxy class may be a newly defined dynamic module per the given 
> class loader.  So a given list of proxy interfaces + the defining class 
> loader will map to one Module.
> 

Ok, so it takes some additional arguments to compute the value, so does not map 
quite so cleanly.



>> 
>> sun.invoke.util.VerifyAccess
>> —
>> 
>> 198 while (c.isArray()) {
>> 199 c = c.getComponentType();
>> 200 }
>> 
>> This pattern is occurring a number of times.
>> 
>> Memo: add a method on Arrays, or somewhere…,
>> 
> 
> +1.  Perhaps Class::getBaseElementType.
> 

Yes, something like that.

Thanks,
Paul.

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 11, 2016, at 1:19 PM, Paul Sandoz  wrote:
> 
>> 
>> On 11 Mar 2016, at 21:53, Mandy Chung  wrote:
>> 
>> I agree. It's also used from ResourceBundle.Control::newBundle. Since core 
>> reflection now gets readability for free, setAccessible is no longer needed 
>> [1]. So it can call Class::newInstance instead.  I fixed both places not to 
>> use Unsafe.
>> 
> 
> Thanks!

In fact, I have to keep setAccessible since the type may not exported to 
java.base.  I changed it to use sneaky throw.

> Ok. One advantage of the merged-approach is it’s atomic. The former may 
> update an entry twice if there is a race, but that may not matter.

OK. Good to change it anyway.

In case you want to see the diff:
  
http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jake-m3/webrev-03-11/index.html

Mandy

Re: Initial webrev with changes for JDK 9

[Phil Race]

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/test/java/awt/patchlib/java.desktop/java/awt/Helper.java.html

  30 //java.awt.Helper.class.getModule().addExports(pn, target);

Can we remove the commented out line ?

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/macosx/classes/com/apple/laf/AquaUtils.java.sdiff.html

 368 // can we use ClassLoader.getSystemClassLoader here?
 369 final ClassLoader launcherClassLoader = 
ClassLoaders.appClassLoader();

I suspect yes, although it doesn't really matter and maybe this workaround 
method
is not needed any more but that is for another day.


http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/java/awt/color/ICC_Profile.java.sdiff.html

This edit seems to be associated with the following change :-
changeset:   13355:df27ca47fd34
user:alanb
date:Sun Jul 12 21:24:44 2015 +0100
summary: ICC_Profile.standardProfileExists doesn't close stream

This bug would affected mainline so why was this change not fixed there ?
I'm curious how it was found since there is no bug report mentioned.
Also I wonder if we really have to have to open a stream to test existence ?



http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/javax/swing/text/DefaultFormatter.java.sdiff.html

< 34 import javax.swing.text.*;

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/javax/swing/text/html/ObjectView.java.sdiff.html
<   27 import java.util.Enumeration;

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/sun/applet/AppletSecurity.java.sdiff.html
<  46 import sun.security.provider.*;

Not sure why deleting a few unused imports needed to be part of this change.
It would have been less odd were it not the only change in these source
files.

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/sun/print/ServiceDialog.java.sdiff.html

Why were these changes made in jake ? I thought the policy was to make
all changes in mainline unless they could only be made in jake ?

Also I
1) don't see where/how the stream is closed
2) The doPrivileged at line 2823 no longer seems likely to be necessary.


Looking over the client related tests (or at least as many of them as I 
noticed) I am curious why this one :-

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/test/java/awt/xembed/server/TesterClient.java.sdiff.html

uses

  35 
cl.getModule().addExports("sun.awt.X11",TesterClient.class.getModule());

Instead ofjava.awt.Helper.addExports(..)




http://cr.openjdk.java.net/~alanb/8142968/2/jdk/test/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh.sdiff.html

Why was the "@" removed from this line :-

  31 #   compile TestWrapped.java

  31 #   compile TestWrapped.java

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/test/java/awt/Mixing/AWT_Mixing
/OverlappingTestBase.java.sdiff.html

Why were lines 111&112 commented out but not lines 114 ?

-phil.

On 03/11/2016 01:39 AM, Alan Bateman wrote:


I've refreshed the webrevs here:
   http://cr.openjdk.java.net/~alanb/8142968/2

so that we have a snapshot of what is currently in the jigsaw/jake 
forest. The webrevs are against jdk-9+109.


As I said in the last mail, we would like to integrate this snapshot 
into JDK 9 before the end of March. The proposal is to aim to 
integrate during the week of March 21, with the the week of March 28 
as fallback in event of problems.


The related JEPs (JEP 200, JEP 260, JEP 261 and JEP 282) are now in 
"Proposed to Target" state. Mark send mail to jdk9-dev with this list 
and other proposals yesterday.


-Alan

Re: Initial webrev with changes for JDK 9

[Xueming Shen]

jar.Main: comments

(1) InputstreamSupplier:
 since what we really need here is the byte[], maybe just go straightforward
 to use InputStream/Files.(path)readAllBytes() ?

(2) #273 don't the "moduleInfo" used for consistency check the same one as
 the used for updating at #244? can't be shared?

(3) if it was me I would simply have passed the "moduleInfoBytes" around as
 a byte[], we might not even need this "InputStreamSupplier" interface.

(4) printModuleDescriptor: for a "file" jar, it might be much faster to open the
 zip file with a ZipFile, then entry -> input stream. otherwise, the ZIS 
might
 be very slow if it's a big jar and the descriptor file is at the end of 
the file.

(5) hashDependences:
  "matcher" can be reused as
  Matcher m = dependenciesToHash.matcher("");
  for (...) {
  m.reset(...).find() ...
  }
  btw, what's the spec of the "mach" here? a "match()" or a "find()"? just 
check.

-sherman

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

On 14/03/2016 19:37, Phil Race wrote:

:

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/macosx/classes/com/apple/laf/AquaUtils.java.sdiff.html 



 368 // can we use ClassLoader.getSystemClassLoader here?
 369 final ClassLoader launcherClassLoader = 
ClassLoaders.appClassLoader();


I suspect yes, although it doesn't really matter and maybe this 
workaround method

is not needed any more but that is for another day.
Right, although we haven't really changed anything here. I assume the 
check for com.installshield.wizard.platform.macosx.MacOSXUtils is 
something that came from Apple originally.






http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/java/awt/color/ICC_Profile.java.sdiff.html 



:

This bug would affected mainline so why was this change not fixed there ?
I'm curious how it was found since there is no bug report mentioned.
Also I wonder if we really have to have to open a stream to test 
existence ?
I think we can revert this. There was a time where it wasn't possible to 
get a URL to a resource.




:

Not sure why deleting a few unused imports needed to be part of this 
change.
We had to remove several unused imports in places where it attempted to 
import types from non-exported packages or from modules that are 
declared as a dependency.





:

http://cr.openjdk.java.net/~alanb/8142968/2/jdk/src/java.desktop/share/classes/sun/print/ServiceDialog.java.sdiff.html 



Why were these changes made in jake ? I thought the policy was to make
all changes in mainline unless they could only be made in jake ?
This is also related to URL to resources, I think we can revert this one 
too.





Also I
1) don't see where/how the stream is closed
2) The doPrivileged at line 2823 no longer seems likely to be necessary.
BTW: try (in)  { .. } will ensure that the input stream is closed. Also 
the doPrivileged is needed as there may be user code on the stack.



I'll leave it to Yuri for the AWT tests as I think he contributed all 
the changes to the tests.


-Alan.

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 11, 2016, at 1:39 AM, Alan Bateman  wrote:
> 
> 
> I've refreshed the webrevs here:
>   http://cr.openjdk.java.net/~alanb/8142968/2


I have reviewed the jmod tool and some comments:

299 private boolean printModuleDescriptor(InputStream in)

jmod -p option prints the output in different sections.
java -listmods: prints the module descriptor closer to 
module-info.java declaration.  Also jmod -p does not do any 
sorting and names are unordered.

It would be better for both options to use similar format.  I think
closer to how it is declared in module-info.java would be preferred.
The optional attributes will follow it - the existing format is fine.

It’d help if the package names and uses are printed in alphabetical order.

 584 } catch (ZipException x) {
 585 // Skip. Do nothing. No packages will be added

When ZipException is thrown?  Should it be handled in the same way as 
IOException?

 603 .filter(pkg -> pkg.length() > 0)   // module-info

I think jmod should detect if there is any unnamed package and output an error 
since unnamed package is not allowed in named module.  Currently any classes in 
unnamed package are include in the jmod file.

findPackages should filter module-info.class explicitly.

 396 Path tempTarget = target.resolveSibling(target.getFileName() + 
".tmp”);

When any error occurs, foo.mod.tmp is left behind.

jmods.properties - some unused messages.

err.cp.must.be.specified:--class-path must be specified
err.dir.not.empty=not empty: {0}
err.invalid.arg.for.option=invalid argument for option: {0}
err.option.after.class=option must be specified before classes: {0}

Mandy

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

Thanks again for going through all the changes in this area.

I've reverted the changes to java/awt/color/ICC_Profile.java and 
sun/print/ServiceDialog.java as they changes were clearly left over from 
early exploration into resource encapsulation.


I've decided not to touch com/apple/laf/AquaUtils.java as that is 
something that should be cleaned up in jdk9/client. The only reason we 
had to touch this code was because it directly used sun.misc.Launcher, 
which is now gone.


Yuri needs to look at test/java/awt/xembed/server/TesterClient.java as 
that addExports usage is not right. I hope Yuri can also help on your 
comment about why skipTestingEmbeddedFrame=true is commented out in 
test/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java.


test/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh 
could do with a clean-up too, I assume the @compile was dropped because 
the test is invoking javac directly now.


-Alan

Re: Initial webrev with changes for JDK 9

[Chris Hegarty]

Thank you for the feedback Sherman.

On 14/03/16 19:50, Xueming Shen wrote:

jar.Main: comments

(1) InputstreamSupplier:
  since what we really need here is the byte[], maybe just go
straightforward
  to use InputStream/Files.(path)readAllBytes() ?


That is cleaner. Done.


(2) #273 don't the "moduleInfo" used for consistency check the same one as
  the used for updating at #244? can't be shared?


Not always, it can be augmented by addExtendedModuleAttributes .


(3) if it was me I would simply have passed the "moduleInfoBytes" around as
  a byte[], we might not even need this "InputStreamSupplier"
interface.


Similar to 1 above. Done.


(4) printModuleDescriptor: for a "file" jar, it might be much faster to
open the
  zip file with a ZipFile, then entry -> input stream. otherwise,
the ZIS might
  be very slow if it's a big jar and the descriptor file is at the
end of the file.


That is better. Done


(5) hashDependences:
   "matcher" can be reused as
   Matcher m = dependenciesToHash.matcher("");
   for (...) {
   m.reset(...).find() ...
   }


Thanks, Done.


   btw, what's the spec of the "mach" here? a "match()" or a
"find()"? just check.


'find'.

You can find the changes here:
  http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/507b98946557

-Chris.

Re: Initial webrev with changes for JDK 9

[Yuri Nesterenko]

On 03/15/2016 02:22 PM, Alan Bateman wrote:
[...]


Yuri needs to look at test/java/awt/xembed/server/TesterClient.java as
that addExports usage is not right. I hope Yuri can also help on your

^ this one is historic: it is from immemorial July while the Helper
class is dated by December. I could piggyback a change to
the new version of my test fix currently on review.


comment about why skipTestingEmbeddedFrame=true is commented out in
test/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java.

Originally, the tests run twice (second time with EmbeddedFrame)
and reported "do not skipTestingEmbeddedFrame" in between.
Then the Mac port work started, and that second run was skipped for Mac.
Then unskipped.



test/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh
could do with a clean-up too, I assume the @compile was dropped because
the test is invoking javac directly now.

Exactly; it requires now a different set of compile-time flags
for every platform which is hard to pass in @compile. I decided not
to remove it as a sign "avoid this" for a future reader.
But then, what should I clean there? A commented out part?
I could also append a change to the outstanding fix.

-yan

Re: Initial webrev with changes for JDK 9

[Peter Levart]

Hi Alan, Paul,

Just a comment on comment: wouldn't the variant with a single compute 
replace a Package with itself on each repeated call? Semantically it is 
the same, but there is a volatile memory store on the cache-hit involved 
where in the original variant, it isn't...


On 03/11/2016 05:50 PM, Paul Sandoz wrote:

j.l.ClassLoader
—

1777 // define Package object if the named package is not yet defined
1778 NamedPackage p = packages.computeIfAbsent(name,
1779   k -> 
NamedPackage.toPackage(name, m));
1780 if (p instanceof Package)
1781 return (Package)p;
1782
1783 // otherwise, replace the NamedPackage object with Package object
1784 return (Package)packages.compute(name, this::toPackage);

You could merge the computeIfAbsent and compute into a single compute call if 
you so wish.

return (Package)packages.compute((n, p) -> {
   // define Package object if the named package is not yet defined
   if (p == null) return NamedPackage.toPackage(name, m);
   // otherwise, replace the NamedPackage object with Package object
   return (p instanceof Package) ? p : toPackage(n, p);
});


You could pre-screen the .compute with a .get and this would be the more 
optimal (no locking on cache-hits):


NamedPackage p = packages.get(name);
if (p instanceof Package) {
return (Package) p;
}

return (Package)packages.compute((n, p) -> {
  // define Package object if it is not yet defined or replace it if it 
is a NamedPackage

  return (p instanceof Package) ? p : NamedPackage.toPackage(n, m);
});

See, no private ClassLoader.toPackage(String name, NamedPackage p) needed.


If you also care for constant lambda, this could be optimized even 
further, but for the price of more complex code:



NamedPackage p = packages.get(name);

if (p instanceof Package) {
return (Package) p;
} else if (p == null) {
Package pkg = NamedPackage.toPackage(name, m);
p = packages.putIfAbsent(name, pkg);
if (p == null) {
return pkg;
}
}

return (Package)packages.compute((n, p) -> {
assert p != null;
// replace NamedPackage with Package
return (p instanceof Package) ? p : 
NamedPackage.toPackage(p.name(), p.module());

});


Regards, Peter

Re: Initial webrev with changes for JDK 9

[Peter Levart]

Sorry,

On 03/15/2016 02:56 PM, Peter Levart wrote:
If you also care for constant lambda, this could be optimized even 
further, but for the price of more complex code:



NamedPackage p = packages.get(name);

if (p instanceof Package) {
return (Package) p;
} else if (p == null) {
Package pkg = NamedPackage.toPackage(name, m);
p = packages.putIfAbsent(name, pkg);
if (p == null) {
return pkg;
}
}

return (Package)packages.compute((n, p) -> {


return (Package)packages.compute(name, (n, p) -> {


assert p != null;
// replace NamedPackage with Package
return (p instanceof Package) ? p : 
NamedPackage.toPackage(p.name(), p.module());
}); 

Re: Initial webrev with changes for JDK 9

[Daniel Fuchs]

Hi Alan,

I had a look at the jdeps changes and they look good.
I have a couple of minor comments:


http://cr.openjdk.java.net/~alanb/8142968/2/langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/Archive.java.frames.html

120 @Override
121 public int hashCode() {
122 int hash = 7;
123 hash = 67*hash + Objects.hashCode(this.filename) +
124 Objects.hashCode(this.path);
125 return hash;
126 }

I wonder if that could be simplified in:

return Objects.hash(this.filename, this.path);


http://cr.openjdk.java.net/~alanb/8142968/2/langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsTask.java.frames.html

typo:
443 // otherwise analyze the depednencies

best regards,

-- daniel
On 11/03/16 10:39, Alan Bateman wrote:


I've refreshed the webrevs here:
http://cr.openjdk.java.net/~alanb/8142968/2

so that we have a snapshot of what is currently in the jigsaw/jake
forest. The webrevs are against jdk-9+109.

As I said in the last mail, we would like to integrate this snapshot
into JDK 9 before the end of March. The proposal is to aim to integrate
during the week of March 21, with the the week of March 28 as fallback
in event of problems.

The related JEPs (JEP 200, JEP 260, JEP 261 and JEP 282) are now in
"Proposed to Target" state. Mark send mail to jdk9-dev with this list
and other proposals yesterday.

-Alan

Re: Initial webrev with changes for JDK 9

[Chris Hegarty]

Mandy,

On 14 Mar 2016, at 20:37, Mandy Chung  wrote:

> 
>> On Mar 11, 2016, at 1:39 AM, Alan Bateman  wrote:
>> 
>> 
>> I've refreshed the webrevs here:
>>  http://cr.openjdk.java.net/~alanb/8142968/2
> 
> 
> I have reviewed the jmod tool and some comments:
> 
> 299 private boolean printModuleDescriptor(InputStream in)
> 
> jmod -p option prints the output in different sections.
> java -listmods: prints the module descriptor closer to 
> module-info.java declaration.  Also jmod -p does not do any 
> sorting and names are unordered.
> 
> It would be better for both options to use similar format.  I think
> closer to how it is declared in module-info.java would be preferred.
> The optional attributes will follow it - the existing format is fine.

Good idea. I updated the output as close as possible, where applicable,
to -listmods:.

> It’d help if the package names and uses are printed in alphabetical order.
> 
> 584 } catch (ZipException x) {
> 585 // Skip. Do nothing. No packages will be added
> 
> When ZipException is thrown?  Should it be handled in the same way as 
> IOException?

I do remember adding this explicit catch. I’m reluctant to remove it
until I can find my notes, as to why it was added. I’ll have to get back
to you on this.

> 603 .filter(pkg -> pkg.length() > 0)   // module-info
> 
> I think jmod should detect if there is any unnamed package and output an 
> error since unnamed package is not allowed in named module.  Currently any 
> classes in unnamed package are include in the jmod file.

Classes in the unnamed package are now disallowed.

> findPackages should filter module-info.class explicitly.
> 
> 396 Path tempTarget = target.resolveSibling(target.getFileName() + 
> ".tmp”);
> 
> When any error occurs, foo.mod.tmp is left behind.

Fixed.

> jmods.properties - some unused messages.
> 
> err.cp.must.be.specified:--class-path must be specified
> err.dir.not.empty=not empty: {0}
> err.invalid.arg.for.option=invalid argument for option: {0}
> err.option.after.class=option must be specified before classes: {0}

Removed.

Changeset with the above updates:
  http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/7e5d2398a250

-Chris.

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

Daniel,

Thanks for the review.

> On Mar 15, 2016, at 10:48 AM, Daniel Fuchs  wrote:
> 
> 
> 120 @Override
> 121 public int hashCode() {
> 122 int hash = 7;
> 123 hash = 67*hash + Objects.hashCode(this.filename) +
> 124 Objects.hashCode(this.path);
> 125 return hash;
> 126 }
> 
> I wonder if that could be simplified in:
> 
> return Objects.hash(this.filename, this.path);
> 

Good idea.  I made the change.

> 
> typo:
> 443 // otherwise analyze the depednencies


Fixed.

Mandy

Re: Initial webrev with changes for JDK 9

[Bhavesh Patel]

Hi,
I have review the javadoc (including doclet and tests) changes. It 
mostly looks good. Following are my comments on the changes


1) jdk/javadoc/internal/doclets/formats/html/ConfigurationImpl.java

- (Line 522) PackageSymbol "pd" is cast to ModuleSymbol which is incorrect.

2) jdk/javadoc/internal/doclets/toolkit/AbstractDoclet.java

- (Lines 109 and 111) e.printStackTrace() needs to be deleted.

3) jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java

- (Lines 130 - 135) The Module label and name gets enclosed in . We 
need to update this to enclose them in a  and  similar to 
what we do in 
jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java (Lines 
200 - 205).


Other minor changes that are needed are as follows

4) com/sun/tools/doclets/formats/html/PackageWriterImpl.java

- The class lists no changes other than the copyright year change. If no 
changes have happened, the copyright year change needs to be reverted.


5) com/sun/tools/doclets/internal/toolkit/taglets/TagletManager.java

- (Line 275) Missing @param.
- (Line 292) Missing documentation.

6) com/sun/tools/doclets/internal/toolkit/util/DocFileFactory.java

- Copyright year shows 2012 instead of 2016.

7) com/sun/tools/javadoc/DocletInvoker.java

- (Line 367) Missing @param.
- (Line 384) Missing documentation.

8) jdk/javadoc/internal/doclets/formats/html/WriterFactoryImpl.java

- (Lines 94 - 102) needs to be deleted.

9) jdk/javadoc/internal/doclets/toolkit/AbstractDoclet.java

- Line 121 needs to be deleted

10) jdk/javadoc/internal/doclets/toolkit/Configuration.java

- Extra line 398 needs to be deleted.
- (Line 654) Not updated as a part of this fix but @return needs to be 
updated.


11) jdk/javadoc/internal/doclets/toolkit/WriterFactory.java

- (Line 74) The param name should be "mdle" instead of "module"

12) jdk/javadoc/internal/doclets/toolkit/builders/BuilderFactory.java

- (Line 108) The param name should be "mdle" instead of "module"

13) jdk/javadoc/internal/doclets/toolkit/taglets/TagletManager.java

- (Line 266) Missing @param.
- (Line 285) Missing documentation.

14) jdk/javadoc/internal/doclets/toolkit/util/DocPaths.java

- (Line 158) should be "The name of the file for the module overview frame."

15) jdk/javadoc/internal/tool/Start.java

- (Line 313) Missing @param.

16) jdk/javadoc/internal/doclets/toolkit/builders/ModuleSummaryBuilder.java

- (Line 80) "module" should be "mdle".
- (Line 95) "module" should be "mdle".

Regards,
Bhavesh.

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

I've refreshed the webrevs here:
http://cr.openjdk.java.net/~alanb/8142968/3

so that we have an up to date snapshot of what is currently in the 
jigsaw/jake forest. The webrevs are against jdk-9+110.


Thank you to the many people that have been helping with the review and 
addressing comments/issues quickly. Overall, I think we are converging 
to the bits that we will push to jdk9/jdk9. As expected, there are a few 
issues where some re-work is needed and those issues will be tracked in 
JIRA to avoid destabilizing things now.


So at this point then I think we are on track to integrate into JDK 9 
next week for jdk-9+111. If something serious comes up then we might 
have to change that plan and go for the week after that, I hope not. I 
sent a note to jdk9-dev last week [1] so that the wider project knows 
what is coming. I will send another note later in the week once 
confidence is higher about next week.


In terms of logistics then we have created jigsaw/m3 as a staging 
forest. We used jigsaw/m1 to stage JEP 201 and jigsaw/m2 to stage JEP 
220 so this is a similar deal except jigsaw/m3 has jcheck configured to 
only accept change-sets that can be pushed to JDK 9. There will be 
activity in this new staging forest for the next few weeks, the 
notifications go to this mailing list so apologies for the spam.


-Alan.

[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-March/003877.html

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 16, 2016, at 1:04 PM, Peter Levart  wrote:
> 
> Hi Alan,
> 
> On 03/16/2016 09:30 AM, Alan Bateman wrote:
>> I've refreshed the webrevs here:
>> http://cr.openjdk.java.net/~alanb/8142968/3
> 
> I have another optimization...
> 
> In java.lang.reflect.Proxy, a package is added dynamically to the module the 
> 1st time a proxy class is defined in that module. Each time new proxy class 
> is defined, an array of module package names is compiled by concatenating two 
> Streams, dumping them into array, wrapping it with an ArrayStream and 
> searching for package if it is already defined:
> 
> 583 // add the package to the runtime module if not exists
> 584 if (m.isNamed() && 
> !Stream.of(m.getPackages()).anyMatch(proxyPkg::equals)) {
> 585 m.addPackage(proxyPkg);
> 586 }
> 
> ... just to avoid calling Module.addPackage() in case the package is already 
> defined in that module, although the Module.addPackage() is idempotent. 
> Presumably to avoid synchronization? But if the module has lots of packages, 
> then such linear search is expensive and produces garbage.
> 
> Here's how this linear search and the synchronization in Module.addPackage() 
> can be avoided:
> 
> http://cr.openjdk.java.net/~plevart/jake/Proxy.addPackage.opt/webrev.01/

I’ll sponsor this patch.

Mandy

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 16, 2016, at 11:50 AM, Mandy Chung  wrote:
> 
>> On Mar 16, 2016, at 10:30 AM, Peter Levart  wrote:
>> 
>> In java.lang.ClassLoader:
>> 
>> ...the package-private definePackage(String name, Module m) is OK to use a 
>> single packages.compute(...) call performance-wise since it is pre-screened 
>> with packages.get() in public getDefinedPackage(String name) method.

Peter - thanks for clarifying this.  This suggests no need to change 
definePackage(String,Module).

>> But there's also a package-private packages() method (a basis for public 
>> methods getPackages() and getDefinedPackages()) that constructs a 
>> Stream of defined Packages which unnecessarily calls 
>> definePackage() for each value of packages map:
>> 
>>   Stream packages() {
>>   return packages.values().stream()
>>  .map(p -> definePackage(p.packageName(), p.module()));
>>   }
>> 
>> 
>> It would be nice performance-wise to avoid calling definePackage if the 
>> value is already a Package:
>> 
>>   Stream packages() {
>>   return packages.values().stream()
>>  .map(p -> p instanceof Package
>>? (Package) p
>>: definePackage(p.packageName(), p.module()));
>>   }
>> 

I have cleaned up the code to use toPackage instead:
  
http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jigsaw-m3/webrev-03-16/index.html

Mandy

Re: Initial webrev with changes for JDK 9

[Peter Levart]

Hi Alan,

On 03/16/2016 09:30 AM, Alan Bateman wrote:

I've refreshed the webrevs here:
http://cr.openjdk.java.net/~alanb/8142968/3


I have another optimization...

In java.lang.reflect.Proxy, a package is added dynamically to the module 
the 1st time a proxy class is defined in that module. Each time new 
proxy class is defined, an array of module package names is compiled by 
concatenating two Streams, dumping them into array, wrapping it with an 
ArrayStream and searching for package if it is already defined:


 583 // add the package to the runtime module if not exists
 584 if (m.isNamed() && 
!Stream.of(m.getPackages()).anyMatch(proxyPkg::equals)) {

 585 m.addPackage(proxyPkg);
 586 }

... just to avoid calling Module.addPackage() in case the package is 
already defined in that module, although the Module.addPackage() is 
idempotent. Presumably to avoid synchronization? But if the module has 
lots of packages, then such linear search is expensive and produces garbage.


Here's how this linear search and the synchronization in 
Module.addPackage() can be avoided:


http://cr.openjdk.java.net/~plevart/jake/Proxy.addPackage.opt/webrev.01/


Regards, Peter

Re: Initial webrev with changes for JDK 9

[Alan Bateman]

On 16/03/2016 20:04, Peter Levart wrote:

:

I have another optimization...

In java.lang.reflect.Proxy, a package is added dynamically to the 
module the 1st time a proxy class is defined in that module. Each time 
new proxy class is defined, an array of module package names is 
compiled by concatenating two Streams, dumping them into array, 
wrapping it with an ArrayStream and searching for package if it is 
already defined:


 583 // add the package to the runtime module if not exists
 584 if (m.isNamed() && 
!Stream.of(m.getPackages()).anyMatch(proxyPkg::equals)) {

 585 m.addPackage(proxyPkg);
 586 }

... just to avoid calling Module.addPackage() in case the package is 
already defined in that module, although the Module.addPackage() is 
idempotent. Presumably to avoid synchronization? But if the module has 
lots of packages, then such linear search is expensive and produces 
garbage.


Here's how this linear search and the synchronization in 
Module.addPackage() can be avoided:


http://cr.openjdk.java.net/~plevart/jake/Proxy.addPackage.opt/webrev.01/

Ugh, I haven't noticed that ProxyClassFactory defineProxyClass was doing 
a linear search, that's a consequence of getPackages() returning an 
array. So I think what you have make sense - thanks!


-Alan

Re: Initial webrev with changes for JDK 9

[Peter Levart]

Hi Mandy,

On 03/16/2016 10:03 PM, Mandy Chung wrote:

I have cleaned up the code to use toPackage instead:
   
http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jigsaw-m3/webrev-03-16/index.html

Mandy


Was your intention for public methods getDefinedPackage(name), 
getPackages() and getDefinedPackages() to return transient Package 
instances in cases where NamedPackage(s) are present in 'packages' map? 
Previously, NamedPackage(s) were replaced with Package(s) in 'packages' 
map when 1st requested. The toPackage(String, NamedPackage, Module) does 
not replace - it just creates new Package instance if needed.


Perhaps we need a method like:

private Package definePackageIfNeeded(String name, Module m, /* nullable 
*/ NamedPackage np) {

return (np instanceof Package)
? (Package) np
: packages.compute(name, (n, p) ->
  (p instanceof Package)
  ? (Package) p
  : NamedPackage.toPackage(n, m));
}


Then we can use it everywhere:

public final Package getDefinedPackage(String name) {
NamedPackage p = packages.get(name);
return (p == null) ? null : 
definePackageIfNeeded(p.packageName(), p.module(), p);

}

Stream packages() {
return packages.values().stream()
   .map(p -> definePackageIfNeeded(p.packageName(), 
p.module(), p));

}

Package definePackage(String name, Module m) {
if (name.isEmpty() && m.isNamed()) {
throw new InternalError("unnamed package in  " + m);
}

return definePackageIfNeeded(name, m, null);
}



What do you think?


Regards, Peter

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

Hi Peter,

Thanks for the review feedback.

> 
> NamedPackage p = packages.get(name);
> if (p instanceof Package) {
>return (Package) p;
> }
> 
> return (Package)packages.compute((n, p) -> {
>  // define Package object if it is not yet defined or replace it if it is a 
> NamedPackage
>  return (p instanceof Package) ? p : NamedPackage.toPackage(n, m);
> });
> 
> See, no private ClassLoader.toPackage(String name, NamedPackage p) needed.
> 

I like this suggestion that allows me to remove this private method.  I have 
this patch:
   http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jigsaw-m3/webrev-03-16/

One thing to note is that I don’t view definePackage returning Package object 
is performance critical.

java.lang.Package is legacy and from my scan of its usage, it's mainly used for 
getting the package name.  The versioning info is more for tracing/debugging 
use.  Package objects has not been defined for every packages.  Not to mention 
its assumption on class loader hierarchy and  packages are not splitted among 
JAR files.

In jake, Alex and I rewrote the java.lang.Package class spec.  Also fixed 
several design issue of Package.  I’d recommend any use of 
Class.getPackage().getName() be replaced with Class::getPackageName which is 
more performant if performance is important to them.  Package annotation 
(package-info) is the main useful info to be obtained from Package object. 
Otherwise I am not aware anything in Package class needed to be performant.

> 
> If you also care for constant lambda, this could be optimized even further, 
> but for the price of more complex code:

Simple clean code as above is good for this case.

> On Mar 16, 2016, at 10:30 AM, Peter Levart  wrote:
> 
> In java.lang.ClassLoader:
> 
> ...the package-private definePackage(String name, Module m) is OK to use a 
> single packages.compute(...) call performance-wise since it is pre-screened 
> with packages.get() in public getDefinedPackage(String name) method. But 
> there's also a package-private packages() method (a basis for public methods 
> getPackages() and getDefinedPackages()) that constructs a Stream of 
> defined Packages which unnecessarily calls definePackage() for each value of 
> packages map:
> 
>Stream packages() {
>return packages.values().stream()
>   .map(p -> definePackage(p.packageName(), p.module()));
>}
> 
> 
> It would be nice performance-wise to avoid calling definePackage if the value 
> is already a Package:
> 
>Stream packages() {
>return packages.values().stream()
>   .map(p -> p instanceof Package
> ? (Package) p
> : definePackage(p.packageName(), p.module()));
>}
> 

As explained above, getting a Package object is not performance critical. I’ll 
keep this in mind for other performance critical code.

thanks
Mandy

Re: Initial webrev with changes for JDK 9

[Mandy Chung]

> On Mar 16, 2016, at 3:18 PM, Peter Levart  wrote:
> 
> Hi Mandy,
> 
> On 03/16/2016 10:03 PM, Mandy Chung wrote:
>> I have cleaned up the code to use toPackage instead:
>>   
>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jigsaw-m3/webrev-03-16/index.html
>> 
>> 
>> Mandy
>> 
> 

Please see the updated webrev that essentially achieve the same result as you 
propose.  

Mandy

Re: Initial webrev with changes for JDK 9

[Peter Levart]

Hi Alan,

Just one nit.

On 03/16/2016 09:30 AM, Alan Bateman wrote:


I've refreshed the webrevs here:
http://cr.openjdk.java.net/~alanb/8142968/3



In java.lang.ClassLoader:

...the package-private definePackage(String name, Module m) is OK to use 
a single packages.compute(...) call performance-wise since it is 
pre-screened with packages.get() in public getDefinedPackage(String 
name) method. But there's also a package-private packages() method (a 
basis for public methods getPackages() and getDefinedPackages()) that 
constructs a Stream of defined Packages which unnecessarily 
calls definePackage() for each value of packages map:


Stream packages() {
return packages.values().stream()
   .map(p -> definePackage(p.packageName(), 
p.module()));

}


It would be nice performance-wise to avoid calling definePackage if the 
value is already a Package:


Stream packages() {
return packages.values().stream()
   .map(p -> p instanceof Package
 ? (Package) p
 : definePackage(p.packageName(), 
p.module()));

}


Regards, Peter

Re: Initial webrev with changes for JDK 9 - jimage

[Roger Riggs]

Hi,

Review comments for the jimage code in java.base, mostly cleanup but 
perhaps a bug or two.
General:  inconsistent style and not following guidelines, copyright 
dates may need an update.
Use of assert for error checking is not going to catch or help isolate 
errors in production builds.


dir: jdk/src/java.base/share/classes/jdk/internal/jimage/


BasicImageReader:
- readHeader() exception message should say not a jimage or wrong version
 Other IOExceptions can occur if the file is corrupted or is too small, 
array index out of bounds, etc.


- findLocation() should use a more explicit computation for the redirection
  than rehashing; this would allow the hash seed to be encapsulated.

- slice() synchronizes on the ByteBuffer; does every other access?

- getAllLocations() is unused and does not respect its sorted argument;
  Best to remove the method for now.

- getResourceBuffer(): unused - remove

- getResourceStream(): unused - remove

ImageStringsReader:

- Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
  re-use and better performance.  Alternatively, Modifed UTF-8 should be
  added as a supported encoding.

- The ImageStringsReader.hashCode() should be defined so it can take 
advantage of 32 or 64 bit accesses; since it is used frequently. And it 
needs to be stable across revisions that may be accessing jimages from 
different JDK versions.



- stringFromMUTF8(byte[]) - there may be some optimization to avoid 
expanding ascii to chars and then re-encoding in String.


- charsFromByteBufferLength/charsFromByteBuffer should throw an 
exception if no terminating null, not just when assertions are enabled


- inconsistent assert errors vs exceptions for malformed mutf8.
  Use of assert will not detect errors in production builds

- mutf8FromChars allocates a new working buffer (byte[8]) potentially 
for every UNICODE char.


- mutf8FromString: should take advantage of new String(bytes, offset, 
length, charsetName) to avoid extra allocations.

  (would need a Modified utf-8 charset).

- hashCode(byte[0, seed) should be private; the hashcode algorithm 
should not be variable.



ImageLocation:
 - Merge ImageLocation functions into ImageLocationBase and rename.
   ImageLocation does not have enough unique functionality to be a 
separate class.


- It is a poor design to create an instance that may not be valid.
  It would be better to have an ImageLocation factory method that 
encapsulated the creation and checking



ImageLocationBase:
 - Failing asserts will not cause a runtime error in production.
   But will degenerate into unpredictable other exceptions

ImageStream:
- compress(): Too heavyweight an object to be used and discarded just 
for compress / decompress of small values.



ImageBufferCache:
 - Comments on each method would make the intent clear

 - getBuffer: mixing for loop and explicit indexes is fragile

 - releaseBuffer: check buffer.capacity before threadLocal.get() for 
early return

 - Logic of i,j is flawed, i is never changed; j is/becomes 0
   The first buffer is removed if there are more than max
   Its not clear what algorithm was intended.
   A buffer that is in use can be removed.

 - ImageBufferCache constructor: the isUsed flag should be set in 
getBuffer()

   when it is added to the list (not in the constructor)
   It keeps it consistent with when it is set for a re-used buffer.

ImageHeader:
- BADMAGIC is not used

- readFrom could throw BufferUnderflowException - undocumented

ImageModuleData:
- ImageModuleData seems to be unused;  allModuleNames() is unreferenced


ImageReader:
- ImageReader(path): unused - remove

- Use of assert is non-reassuring, ineffective in production builds

- In a long running application the per thread buffers will get to the 
point wehere they are unused and should be released.

  There is no support for soft or weak references to handle that case.
  Alternatively, if the buffers were not per thread then they would 
have a lesser impact.

  Also, why should there be more than one buffer per thread?

ImageReader.Resource:
- unused create method

- unused parent argument (0) in Resource constructor (dir, loc, attr)

ImageReader.LinkNode:
- constructor has unused parent arg


decompress/CompressIndexes:
- readInt: wasteful allocation of a single byte array and then another 
small byte array;

  and then another heap buffer allocation in the ByteBuffer.

- decompress is hugely wasteful of allocations of ByteBuffers of small 
sizes.


decompress/CompressedResourceHeader:
- MAGIC does not need to be public


decompress/Decompressor:
- IOException "Plugin name not found" should include the missing name

- StringsProvider is an unnecessary interface; could use Supplier

- Redundant public modifiers in interface declarations

decompress/SignatureParser:
- Comments describing the input and output formats needed

- style around operators does not consistently include spaces; and "if("...

decompress/StringSharingDecompressor

Re: Initial webrev with changes for JDK 9 - jimage

[Alan Bateman]

On 08/03/2016 18:07, Roger Riggs wrote:

Hi,

Review comments for the jimage code in java.base, mostly cleanup but 
perhaps a bug or two.
General:  inconsistent style and not following guidelines, copyright 
dates may need an update.
Use of assert for error checking is not going to catch or help isolate 
errors in production builds.

I think Jim Laskey is going to reply to you on these comments.

On copyright dates then I submitted an issue a few weeks ago to schedule 
a bulk update in JDK 9. This hasn't been done for some time and there 
are many files that need their dates updated.


-Alan

Re: Initial webrev with changes for JDK 9 - jimage

[Jim Laskey (Oracle)]

Thank you Roger.  Comments inline.  In general, I will hold off changes until 
after merge so as not to destabilize.  Note that ImageBufferCache and 
Decompression are not currently used.

> On Mar 8, 2016, at 2:07 PM, Roger Riggs  wrote:
> 
> Hi,
> 
> Review comments for the jimage code in java.base, mostly cleanup but perhaps 
> a bug or two.
> General:  inconsistent style and not following guidelines, copyright dates 
> may need an update.
> Use of assert for error checking is not going to catch or help isolate errors 
> in production builds.

Noted.

> 
> dir: jdk/src/java.base/share/classes/jdk/internal/jimage/
> 
> 
> BasicImageReader:
> - readHeader() exception message should say not a jimage or wrong version
>  Other IOExceptions can occur if the file is corrupted or is too small, array 
> index out of bounds, etc.

Replaced with specific exceptions.

> 
> - findLocation() should use a more explicit computation for the redirection
>   than rehashing; this would allow the hash seed to be encapsulated.

As part of the review change set, I've documented the Minimal Perfect Hash 
Method (see PerfectHashBuilder.)  The algorithm requires a one time one rehash 
when there is a collision.  A new seed forces a different distribution of the 
colliding strings.  If you started with the existing hashCode, you would not 
get a new distribution, merely a shift of all the colliding elements. Random 
distribution is the essence of algorithm.

> 
> - slice() synchronizes on the ByteBuffer; does every other access?

Byte buffer use here is immutable, however slicing of the ByteBuffer does 
require modifying position and limit.  Wrapping or cloning the buffers would be 
of little benefit and would take a performance hit.  The ByteBuffer API might 
have been well served with an atomic non-modifying slice operation.

> 
> - getAllLocations() is unused and does not respect its sorted argument;
>   Best to remove the method for now.

Removed.

> 
> - getResourceBuffer(): unused - remove

Removed.

> 
> - getResourceStream(): unused - remove

Removed.

> 
> ImageStringsReader:
> 
> - Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
>   re-use and better performance.  Alternatively, Modified UTF-8 should be
>   added as a supported encoding.

Modified UTF-8 is used to be more compact and to simplify use in native code.  
The jimage string table has overlapping use with constant pool compaction so is 
really an older revision of the Modified UTF-8 standard.  Not sure why Modifed 
UTF-8 is not offered as an encoding.  It would have made constant pool 
management easier. And note that relying on a particular encoding is 
problematic.  Encoding standards do change, affecting the stability of the hash 
algorithm and may prevent the reading of older jimage files.

> 
> - The ImageStringsReader.hashCode() should be defined so it can take 
> advantage of 32 or 64 bit accesses; since it is used frequently.  And it 
> needs to be stable across revisions that may be accessing jimages from 
> different JDK versions.
> 

Reasonable and will consider.  Not sure what you mean by stable across 
revisions. This is not string hashCode.  This is a hashCode that is specific to 
MPHM and is deterministic. 

> - stringFromMUTF8(byte[]) - there may be some optimization to avoid expanding 
> ascii to chars and then re-encoding in String.

Will touch base with the Aleksey.

> 
> - charsFromByteBufferLength/charsFromByteBuffer should throw an exception if 
> no terminating null, not just when assertions are enabled

Will throw InternalError instead.

> 
> - inconsistent assert errors vs exceptions for malformed mutf8.
>   Use of assert will not detect errors in production builds

Will throw InternalError instead.

> 
> - mutf8FromChars allocates a new working buffer (byte[8]) potentially for 
> every UNICODE char.

Will hoist the buffer allocation and reuse.

> 
> - mutf8FromString: should take advantage of new String(bytes, offset, length, 
> charsetName) to avoid extra allocations.
>   (would need a Modified utf-8 charset).

Modified UTF-8 not available.

> 
> - hashCode(byte[0, seed) should be private; the hashcode algorithm should not 
> be variable.
> 

This is not String hashCode. see above.

> ImageLocation:
>  - Merge ImageLocation functions into ImageLocationBase and rename.
>ImageLocation does not have enough unique functionality to be a separate 
> class.

Merged.

> 
> - It is a poor design to create an instance that may not be valid.
>   It would be better to have an ImageLocation factory method that 
> encapsulated the creation and checking
> 

Noted.

>
> ImageLocationBase:
>  - Failing asserts will not cause a runtime error in production.   
>But will degenerate into unpredictable other exceptions

Will throw InternalError instead.

> 
> ImageStream:
> - compress(): Too heavyweight an object to be used and discarded just for 
> compress / decompress of small values.

compress()?   I don’t see ImageSt

Re: Initial webrev with changes for JDK 9 - jimage

[Roger Riggs]

Thanks Jim.

Beware of use of Lambdas, jimage may be used early enough in the boot 
cycle that lambdas
are not yet fully operative.   The failure is pretty obvious but can be 
trial and error to narrow it down.


It would be useful to create an jira entry for the changes that are 
being delayed.


Roger


On 3/10/2016 6:22 PM, Jim Laskey (Oracle) wrote:

Thank you Roger.  Comments inline.  In general, I will hold off changes until 
after merge so as not to destabilize.  Note that ImageBufferCache and 
Decompression are not currently used.


On Mar 8, 2016, at 2:07 PM, Roger Riggs  wrote:

Hi,

Review comments for the jimage code in java.base, mostly cleanup but perhaps a 
bug or two.
General:  inconsistent style and not following guidelines, copyright dates may 
need an update.
Use of assert for error checking is not going to catch or help isolate errors 
in production builds.

Noted.


dir: jdk/src/java.base/share/classes/jdk/internal/jimage/


BasicImageReader:
- readHeader() exception message should say not a jimage or wrong version
  Other IOExceptions can occur if the file is corrupted or is too small, array 
index out of bounds, etc.

Replaced with specific exceptions.


- findLocation() should use a more explicit computation for the redirection
   than rehashing; this would allow the hash seed to be encapsulated.

As part of the review change set, I've documented the Minimal Perfect Hash 
Method (see PerfectHashBuilder.)  The algorithm requires a one time one rehash 
when there is a collision.  A new seed forces a different distribution of the 
colliding strings.  If you started with the existing hashCode, you would not 
get a new distribution, merely a shift of all the colliding elements. Random 
distribution is the essence of algorithm.


- slice() synchronizes on the ByteBuffer; does every other access?

Byte buffer use here is immutable, however slicing of the ByteBuffer does 
require modifying position and limit.  Wrapping or cloning the buffers would be 
of little benefit and would take a performance hit.  The ByteBuffer API might 
have been well served with an atomic non-modifying slice operation.


- getAllLocations() is unused and does not respect its sorted argument;
   Best to remove the method for now.

Removed.


- getResourceBuffer(): unused - remove

Removed.


- getResourceStream(): unused - remove

Removed.


ImageStringsReader:

- Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
   re-use and better performance.  Alternatively, Modified UTF-8 should be
   added as a supported encoding.

Modified UTF-8 is used to be more compact and to simplify use in native code.  
The jimage string table has overlapping use with constant pool compaction so is 
really an older revision of the Modified UTF-8 standard.  Not sure why Modifed 
UTF-8 is not offered as an encoding.  It would have made constant pool 
management easier. And note that relying on a particular encoding is 
problematic.  Encoding standards do change, affecting the stability of the hash 
algorithm and may prevent the reading of older jimage files.


- The ImageStringsReader.hashCode() should be defined so it can take advantage 
of 32 or 64 bit accesses; since it is used frequently.  And it needs to be 
stable across revisions that may be accessing jimages from different JDK 
versions.


Reasonable and will consider.  Not sure what you mean by stable across 
revisions. This is not string hashCode.  This is a hashCode that is specific to 
MPHM and is deterministic.


- stringFromMUTF8(byte[]) - there may be some optimization to avoid expanding 
ascii to chars and then re-encoding in String.

Will touch base with the Aleksey.


- charsFromByteBufferLength/charsFromByteBuffer should throw an exception if no 
terminating null, not just when assertions are enabled

Will throw InternalError instead.


- inconsistent assert errors vs exceptions for malformed mutf8.
   Use of assert will not detect errors in production builds

Will throw InternalError instead.


- mutf8FromChars allocates a new working buffer (byte[8]) potentially for every 
UNICODE char.

Will hoist the buffer allocation and reuse.


- mutf8FromString: should take advantage of new String(bytes, offset, length, 
charsetName) to avoid extra allocations.
   (would need a Modified utf-8 charset).

Modified UTF-8 not available.


- hashCode(byte[0, seed) should be private; the hashcode algorithm should not 
be variable.


This is not String hashCode. see above.


ImageLocation:
  - Merge ImageLocation functions into ImageLocationBase and rename.
ImageLocation does not have enough unique functionality to be a separate 
class.

Merged.


- It is a poor design to create an instance that may not be valid.
   It would be better to have an ImageLocation factory method that encapsulated 
the creation and checking


Noted.


ImageLocationBase:

  - Failing asserts will not cause a runtime error in production.
But will degenerate into unpredictable 

Re: Initial webrev with changes for JDK 9 - jimage

[Mandy Chung]

We rework the VM initialization such that lambdas can be used very early after 
phase 1 initialization is done and before module system initialization.

I’m not sure where jimage uses of lambdas for this case.  In general, we wanted 
to enable use of new language features early during startup and we have this 
solve in jake (startup performance is a different topic).

Mandy

> On Mar 11, 2016, at 6:27 AM, Roger Riggs  wrote:
> 
> Thanks Jim.
> 
> Beware of use of Lambdas, jimage may be used early enough in the boot cycle 
> that lambdas
> are not yet fully operative.   The failure is pretty obvious but can be trial 
> and error to narrow it down.
> 
> It would be useful to create an jira entry for the changes that are being 
> delayed.
> 
> Roger
> 
> 
> On 3/10/2016 6:22 PM, Jim Laskey (Oracle) wrote:
>> Thank you Roger.  Comments inline.  In general, I will hold off changes 
>> until after merge so as not to destabilize.  Note that ImageBufferCache and 
>> Decompression are not currently used.
>> 
>>> On Mar 8, 2016, at 2:07 PM, Roger Riggs  wrote:
>>> 
>>> Hi,
>>> 
>>> Review comments for the jimage code in java.base, mostly cleanup but 
>>> perhaps a bug or two.
>>> General:  inconsistent style and not following guidelines, copyright dates 
>>> may need an update.
>>> Use of assert for error checking is not going to catch or help isolate 
>>> errors in production builds.
>> Noted.
>> 
>>> dir: jdk/src/java.base/share/classes/jdk/internal/jimage/
>>> 
>>> 
>>> BasicImageReader:
>>> - readHeader() exception message should say not a jimage or wrong version
>>>  Other IOExceptions can occur if the file is corrupted or is too small, 
>>> array index out of bounds, etc.
>> Replaced with specific exceptions.
>> 
>>> - findLocation() should use a more explicit computation for the redirection
>>>   than rehashing; this would allow the hash seed to be encapsulated.
>> As part of the review change set, I've documented the Minimal Perfect Hash 
>> Method (see PerfectHashBuilder.)  The algorithm requires a one time one 
>> rehash when there is a collision.  A new seed forces a different 
>> distribution of the colliding strings.  If you started with the existing 
>> hashCode, you would not get a new distribution, merely a shift of all the 
>> colliding elements. Random distribution is the essence of algorithm.
>> 
>>> - slice() synchronizes on the ByteBuffer; does every other access?
>> Byte buffer use here is immutable, however slicing of the ByteBuffer does 
>> require modifying position and limit.  Wrapping or cloning the buffers would 
>> be of little benefit and would take a performance hit.  The ByteBuffer API 
>> might have been well served with an atomic non-modifying slice operation.
>> 
>>> - getAllLocations() is unused and does not respect its sorted argument;
>>>   Best to remove the method for now.
>> Removed.
>> 
>>> - getResourceBuffer(): unused - remove
>> Removed.
>> 
>>> - getResourceStream(): unused - remove
>> Removed.
>> 
>>> ImageStringsReader:
>>> 
>>> - Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
>>>   re-use and better performance.  Alternatively, Modified UTF-8 should be
>>>   added as a supported encoding.
>> Modified UTF-8 is used to be more compact and to simplify use in native 
>> code.  The jimage string table has overlapping use with constant pool 
>> compaction so is really an older revision of the Modified UTF-8 standard.  
>> Not sure why Modifed UTF-8 is not offered as an encoding.  It would have 
>> made constant pool management easier. And note that relying on a particular 
>> encoding is problematic.  Encoding standards do change, affecting the 
>> stability of the hash algorithm and may prevent the reading of older jimage 
>> files.
>> 
>>> - The ImageStringsReader.hashCode() should be defined so it can take 
>>> advantage of 32 or 64 bit accesses; since it is used frequently.  And it 
>>> needs to be stable across revisions that may be accessing jimages from 
>>> different JDK versions.
>>> 
>> Reasonable and will consider.  Not sure what you mean by stable across 
>> revisions. This is not string hashCode.  This is a hashCode that is specific 
>> to MPHM and is deterministic.
>> 
>>> - stringFromMUTF8(byte[]) - there may be some optimization to avoid 
>>> expanding ascii to chars and then re-encoding in String.
>> Will touch base with the Aleksey.
>> 
>>> - charsFromByteBufferLength/charsFromByteBuffer should throw an exception 
>>> if no terminating null, not just when assertions are enabled
>> Will throw InternalError instead.
>> 
>>> - inconsistent assert errors vs exceptions for malformed mutf8.
>>>   Use of assert will not detect errors in production builds
>> Will throw InternalError instead.
>> 
>>> - mutf8FromChars allocates a new working buffer (byte[8]) potentially for 
>>> every UNICODE char.
>> Will hoist the buffer allocation and reuse.
>> 
>>> - mutf8FromString: should take advantage of new String(bytes, offset, 
>>> length, charsetNa

Re: Initial webrev with changes for JDK 9 - jimage

[Jim Laskey (Oracle)]

A non issue.  I discussed using NIO buffers with Alan.  That is the direction 
we will go.


> On Mar 11, 2016, at 11:36 AM, Mandy Chung  wrote:
> 
> We rework the VM initialization such that lambdas can be used very early 
> after phase 1 initialization is done and before module system initialization.
> 
> I’m not sure where jimage uses of lambdas for this case.  In general, we 
> wanted to enable use of new language features early during startup and we 
> have this solve in jake (startup performance is a different topic).
> 
> Mandy
> 
>> On Mar 11, 2016, at 6:27 AM, Roger Riggs  wrote:
>> 
>> Thanks Jim.
>> 
>> Beware of use of Lambdas, jimage may be used early enough in the boot cycle 
>> that lambdas
>> are not yet fully operative.   The failure is pretty obvious but can be 
>> trial and error to narrow it down.
>> 
>> It would be useful to create an jira entry for the changes that are being 
>> delayed.
>> 
>> Roger
>> 
>> 
>> On 3/10/2016 6:22 PM, Jim Laskey (Oracle) wrote:
>>> Thank you Roger.  Comments inline.  In general, I will hold off changes 
>>> until after merge so as not to destabilize.  Note that ImageBufferCache and 
>>> Decompression are not currently used.
>>> 
 On Mar 8, 2016, at 2:07 PM, Roger Riggs  wrote:
 
 Hi,
 
 Review comments for the jimage code in java.base, mostly cleanup but 
 perhaps a bug or two.
 General:  inconsistent style and not following guidelines, copyright dates 
 may need an update.
 Use of assert for error checking is not going to catch or help isolate 
 errors in production builds.
>>> Noted.
>>> 
 dir: jdk/src/java.base/share/classes/jdk/internal/jimage/
 
 
 BasicImageReader:
 - readHeader() exception message should say not a jimage or wrong version
 Other IOExceptions can occur if the file is corrupted or is too small, 
 array index out of bounds, etc.
>>> Replaced with specific exceptions.
>>> 
 - findLocation() should use a more explicit computation for the redirection
  than rehashing; this would allow the hash seed to be encapsulated.
>>> As part of the review change set, I've documented the Minimal Perfect Hash 
>>> Method (see PerfectHashBuilder.)  The algorithm requires a one time one 
>>> rehash when there is a collision.  A new seed forces a different 
>>> distribution of the colliding strings.  If you started with the existing 
>>> hashCode, you would not get a new distribution, merely a shift of all the 
>>> colliding elements. Random distribution is the essence of algorithm.
>>> 
 - slice() synchronizes on the ByteBuffer; does every other access?
>>> Byte buffer use here is immutable, however slicing of the ByteBuffer does 
>>> require modifying position and limit.  Wrapping or cloning the buffers 
>>> would be of little benefit and would take a performance hit.  The 
>>> ByteBuffer API might have been well served with an atomic non-modifying 
>>> slice operation.
>>> 
 - getAllLocations() is unused and does not respect its sorted argument;
  Best to remove the method for now.
>>> Removed.
>>> 
 - getResourceBuffer(): unused - remove
>>> Removed.
>>> 
 - getResourceStream(): unused - remove
>>> Removed.
>>> 
 ImageStringsReader:
 
 - Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
  re-use and better performance.  Alternatively, Modified UTF-8 should be
  added as a supported encoding.
>>> Modified UTF-8 is used to be more compact and to simplify use in native 
>>> code.  The jimage string table has overlapping use with constant pool 
>>> compaction so is really an older revision of the Modified UTF-8 standard.  
>>> Not sure why Modifed UTF-8 is not offered as an encoding.  It would have 
>>> made constant pool management easier. And note that relying on a particular 
>>> encoding is problematic.  Encoding standards do change, affecting the 
>>> stability of the hash algorithm and may prevent the reading of older jimage 
>>> files.
>>> 
 - The ImageStringsReader.hashCode() should be defined so it can take 
 advantage of 32 or 64 bit accesses; since it is used frequently.  And it 
 needs to be stable across revisions that may be accessing jimages from 
 different JDK versions.
 
>>> Reasonable and will consider.  Not sure what you mean by stable across 
>>> revisions. This is not string hashCode.  This is a hashCode that is 
>>> specific to MPHM and is deterministic.
>>> 
 - stringFromMUTF8(byte[]) - there may be some optimization to avoid 
 expanding ascii to chars and then re-encoding in String.
>>> Will touch base with the Aleksey.
>>> 
 - charsFromByteBufferLength/charsFromByteBuffer should throw an exception 
 if no terminating null, not just when assertions are enabled
>>> Will throw InternalError instead.
>>> 
 - inconsistent assert errors vs exceptions for malformed mutf8.
  Use of assert will not detect errors in production builds
>>> Will throw InternalErro

Re: Initial webrev with changes for JDK 9 - jimage

[Jim Laskey (Oracle)]

These are the changes I plan on submitting for M3.

http://cr.openjdk.java.net/~jlaskey/jimagereview/webrev/index.html

I’ve filed the following bugs as follow up.

https://bugs.openjdk.java.net/browse/JDK-8151806 JImage decompress code needs 
to be revised to be more effective
https://bugs.openjdk.java.net/browse/JDK-8151807 ImageBufferCache should use 
sun.nio.ch.Util

— Jim



> On Mar 10, 2016, at 7:22 PM, Jim Laskey (Oracle)  
> wrote:
> 
> Thank you Roger.  Comments inline.  In general, I will hold off changes until 
> after merge so as not to destabilize.  Note that ImageBufferCache and 
> Decompression are not currently used.
> 
>> On Mar 8, 2016, at 2:07 PM, Roger Riggs  wrote:
>> 
>> Hi,
>> 
>> Review comments for the jimage code in java.base, mostly cleanup but perhaps 
>> a bug or two.
>> General:  inconsistent style and not following guidelines, copyright dates 
>> may need an update.
>> Use of assert for error checking is not going to catch or help isolate 
>> errors in production builds.
> 
> Noted.
> 
>> 
>> dir: jdk/src/java.base/share/classes/jdk/internal/jimage/
>> 
>> 
>> BasicImageReader:
>> - readHeader() exception message should say not a jimage or wrong version
>> Other IOExceptions can occur if the file is corrupted or is too small, array 
>> index out of bounds, etc.
> 
> Replaced with specific exceptions.
> 
>> 
>> - findLocation() should use a more explicit computation for the redirection
>>  than rehashing; this would allow the hash seed to be encapsulated.
> 
> As part of the review change set, I've documented the Minimal Perfect Hash 
> Method (see PerfectHashBuilder.)  The algorithm requires a one time one 
> rehash when there is a collision.  A new seed forces a different distribution 
> of the colliding strings.  If you started with the existing hashCode, you 
> would not get a new distribution, merely a shift of all the colliding 
> elements. Random distribution is the essence of algorithm.
> 
>> 
>> - slice() synchronizes on the ByteBuffer; does every other access?
> 
> Byte buffer use here is immutable, however slicing of the ByteBuffer does 
> require modifying position and limit.  Wrapping or cloning the buffers would 
> be of little benefit and would take a performance hit.  The ByteBuffer API 
> might have been well served with an atomic non-modifying slice operation.
> 
>> 
>> - getAllLocations() is unused and does not respect its sorted argument;
>>  Best to remove the method for now.
> 
> Removed.
> 
>> 
>> - getResourceBuffer(): unused - remove
> 
> Removed.
> 
>> 
>> - getResourceStream(): unused - remove
> 
> Removed.
> 
>> 
>> ImageStringsReader:
>> 
>> - Is Modified UTF-8 really needed here;  using normal UTF-8 would allow
>>  re-use and better performance.  Alternatively, Modified UTF-8 should be
>>  added as a supported encoding.
> 
> Modified UTF-8 is used to be more compact and to simplify use in native code. 
>  The jimage string table has overlapping use with constant pool compaction so 
> is really an older revision of the Modified UTF-8 standard.  Not sure why 
> Modifed UTF-8 is not offered as an encoding.  It would have made constant 
> pool management easier. And note that relying on a particular encoding is 
> problematic.  Encoding standards do change, affecting the stability of the 
> hash algorithm and may prevent the reading of older jimage files.
> 
>> 
>> - The ImageStringsReader.hashCode() should be defined so it can take 
>> advantage of 32 or 64 bit accesses; since it is used frequently.  And it 
>> needs to be stable across revisions that may be accessing jimages from 
>> different JDK versions.
>> 
> 
> Reasonable and will consider.  Not sure what you mean by stable across 
> revisions. This is not string hashCode.  This is a hashCode that is specific 
> to MPHM and is deterministic. 
> 
>> - stringFromMUTF8(byte[]) - there may be some optimization to avoid 
>> expanding ascii to chars and then re-encoding in String.
> 
> Will touch base with the Aleksey.
> 
>> 
>> - charsFromByteBufferLength/charsFromByteBuffer should throw an exception if 
>> no terminating null, not just when assertions are enabled
> 
> Will throw InternalError instead.
> 
>> 
>> - inconsistent assert errors vs exceptions for malformed mutf8.
>>  Use of assert will not detect errors in production builds
> 
> Will throw InternalError instead.
> 
>> 
>> - mutf8FromChars allocates a new working buffer (byte[8]) potentially for 
>> every UNICODE char.
> 
> Will hoist the buffer allocation and reuse.
> 
>> 
>> - mutf8FromString: should take advantage of new String(bytes, offset, 
>> length, charsetName) to avoid extra allocations.
>>  (would need a Modified utf-8 charset).
> 
> Modified UTF-8 not available.
> 
>> 
>> - hashCode(byte[0, seed) should be private; the hashcode algorithm should 
>> not be variable.
>> 
> 
> This is not String hashCode. see above.
> 
>> ImageLocation:
>> - Merge ImageLocation functions into ImageLocationBase and rename.
>>   ImageLocation does

Re: Initial webrev with changes for JDK 9 jrtfs

[Xueming Shen]

(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
 internal storage.

(2) AbstractJrtFilesystem.java

  getPathMatcher:
   
   if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) {
expr = JrtUtils.toRegexPattern(input);
} else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) {
 expr = input;
} else {
throw new UnsupportedOperationException("Syntax '" + syntax
+ "' not recognized");
   }

(3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own copy?

(4) JrtFilesystem.nodesToIterator()

Stream has a "iterator() method. why need a collect() here? different 
performance?
for example

  private Iterator nodesToIterator(AbstractJrtPath dir, List 
childNodes) {
return childNodes.stream()
 .map(child -> 
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
 .iterator();
  }


sherman

Re: Initial webrev with changes for JDK 9 jrtfs

[Xueming Shen]

(5) JrtFileSystemProvider.copy(src, dst, ...options)

The "dst/target" should not be cast to "AbstractJrtPath"? the jrtfs itself is
a readonly, I'm not sure if we want to support the operation of copying
a "file" output jrtfs. The copy/paste "copyToTarget" appears to be functional,
if it's not a "AbstractJrtPath".


On 03/14/2016 04:08 PM, Xueming Shen wrote:

(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
 internal storage.

(2) AbstractJrtFilesystem.java

  getPathMatcher:
   
   if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) {
expr = JrtUtils.toRegexPattern(input);
} else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) {
 expr = input;
} else {
throw new UnsupportedOperationException("Syntax '" + syntax
+ "' not recognized");
   }

(3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own copy?

(4) JrtFilesystem.nodesToIterator()

Stream has a "iterator() method. why need a collect() here? different 
performance?
for example

  private Iterator nodesToIterator(AbstractJrtPath dir, List 
childNodes) {
return childNodes.stream()
 .map(child -> 
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
 .iterator();
  }


sherman

Re: Initial webrev with changes for JDK 9 jrtfs

[Sundararajan Athijegannathan]

Hi,

Thanks for the review. I've filed a bug to track the cleanups
suggested:  https://bugs.openjdk.java.net/browse/JDK-8151860

Quick comments:

1) jrt fs is read-only file system as you noted. Copying content into
jrt fs does not make sense. Eventually read-only file system exception
is thrown.
But, yes that cast can be avoided.

2) jrt-fs.jar is generated from jimage and jrt-fs sources and expected
to work on previous JDK (jdk 8 in this case). This is to support
cross-JDK reference to platform classes (from IDEs).
So, it is better to avoid internal JDK classes such as sun.nio.fs.Globs
in jrt-fs code.

Thanks,
-Sundar

On 3/15/2016 4:51 AM, Xueming Shen wrote:
>
> (5) JrtFileSystemProvider.copy(src, dst, ...options)
>
> The "dst/target" should not be cast to "AbstractJrtPath"? the jrtfs
> itself is
> a readonly, I'm not sure if we want to support the operation of copying
> a "file" output jrtfs. The copy/paste "copyToTarget" appears to be
> functional,
> if it's not a "AbstractJrtPath".
>
>
> On 03/14/2016 04:08 PM, Xueming Shen wrote:
>> (1) JrtFilePath: it does not seem to help performance to use the
>> byte[] as the
>>  internal storage.
>>
>> (2) AbstractJrtFilesystem.java
>>
>>   getPathMatcher:
>>
>>if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) {
>> expr = JrtUtils.toRegexPattern(input);
>> } else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) {
>>  expr = input;
>> } else {
>> throw new UnsupportedOperationException("Syntax '" +
>> syntax
>> + "' not recognized");
>>}
>>
>> (3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own copy?
>>
>> (4) JrtFilesystem.nodesToIterator()
>>
>> Stream has a "iterator() method. why need a collect() here?
>> different performance?
>> for example
>>
>>   private Iterator nodesToIterator(AbstractJrtPath dir,
>> List childNodes) {
>> return childNodes.stream()
>>  .map(child ->
>> (Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
>>  .iterator();
>>   }
>>
>>
>> sherman
>>
>>
>

Re: Initial webrev with changes for JDK 9 jrtfs

[Xueming Shen]

On 3/14/2016 7:48 PM, Sundararajan Athijegannathan wrote:

Hi,

Thanks for the review. I've filed a bug to track the cleanups
suggested:  https://bugs.openjdk.java.net/browse/JDK-8151860

Quick comments:

1) jrt fs is read-only file system as you noted. Copying content into
jrt fs does not make sense. Eventually read-only file system exception
is thrown.
But, yes that cast can be avoided.


Hi Sundar,

What I meant is without that "check&cast-ing", the copyToTarget might
actually work if the "dst/target" is a non-jrtfs-path", for example, copy
a "class" file out of the jrtfs and store into the local file system 
(those code

originally is designed/implemented to work that way, whether or not it
works depends on if the src is readable and if the dst is writable). The
question here is if this is something we want to do for the jrtfs. That is
a design decision to make.

-Sherman





2) jrt-fs.jar is generated from jimage and jrt-fs sources and expected
to work on previous JDK (jdk 8 in this case). This is to support
cross-JDK reference to platform classes (from IDEs).
So, it is better to avoid internal JDK classes such as sun.nio.fs.Globs
in jrt-fs code.

Thanks,
-Sundar

On 3/15/2016 4:51 AM, Xueming Shen wrote:

(5) JrtFileSystemProvider.copy(src, dst, ...options)

The "dst/target" should not be cast to "AbstractJrtPath"? the jrtfs
itself is
a readonly, I'm not sure if we want to support the operation of copying
a "file" output jrtfs. The copy/paste "copyToTarget" appears to be
functional,
if it's not a "AbstractJrtPath".


On 03/14/2016 04:08 PM, Xueming Shen wrote:

(1) JrtFilePath: it does not seem to help performance to use the
byte[] as the
  internal storage.

(2) AbstractJrtFilesystem.java

   getPathMatcher:

if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) {
 expr = JrtUtils.toRegexPattern(input);
 } else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) {
  expr = input;
 } else {
 throw new UnsupportedOperationException("Syntax '" +
syntax
 + "' not recognized");
}

(3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own copy?

(4) JrtFilesystem.nodesToIterator()

 Stream has a "iterator() method. why need a collect() here?
different performance?
 for example

   private Iterator nodesToIterator(AbstractJrtPath dir,
List childNodes) {
 return childNodes.stream()
  .map(child ->
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
  .iterator();
   }


sherman

Re: Initial webrev with changes for JDK 9 jrtfs

[Sundararajan Athijegannathan]

On 3/15/2016 11:24 AM, Xueming Shen wrote:
>
> On 3/14/2016 7:48 PM, Sundararajan Athijegannathan wrote:
>> Hi,
>>
>> Thanks for the review. I've filed a bug to track the cleanups
>> suggested:  https://bugs.openjdk.java.net/browse/JDK-8151860
>>
>> Quick comments:
>>
>> 1) jrt fs is read-only file system as you noted. Copying content into
>> jrt fs does not make sense. Eventually read-only file system exception
>> is thrown.
>> But, yes that cast can be avoided.
>
> Hi Sundar,
>
> What I meant is without that "check&cast-ing", the copyToTarget might
> actually work if the "dst/target" is a non-jrtfs-path", for example, copy
> a "class" file out of the jrtfs and store into the local file system
> (those code
> originally is designed/implemented to work that way, whether or not it
> works depends on if the src is readable and if the dst is writable). The
> question here is if this is something we want to do for the jrtfs.
> That is
> a design decision to make.

That's right. That has to be revisited.

-Sundar

>
> -Sherman
>
>
>
>>
>> 2) jrt-fs.jar is generated from jimage and jrt-fs sources and expected
>> to work on previous JDK (jdk 8 in this case). This is to support
>> cross-JDK reference to platform classes (from IDEs).
>> So, it is better to avoid internal JDK classes such as sun.nio.fs.Globs
>> in jrt-fs code.
>>
>> Thanks,
>> -Sundar
>>
>> On 3/15/2016 4:51 AM, Xueming Shen wrote:
>>> (5) JrtFileSystemProvider.copy(src, dst, ...options)
>>>
>>> The "dst/target" should not be cast to "AbstractJrtPath"? the jrtfs
>>> itself is
>>> a readonly, I'm not sure if we want to support the operation of copying
>>> a "file" output jrtfs. The copy/paste "copyToTarget" appears to be
>>> functional,
>>> if it's not a "AbstractJrtPath".
>>>
>>>
>>> On 03/14/2016 04:08 PM, Xueming Shen wrote:
 (1) JrtFilePath: it does not seem to help performance to use the
 byte[] as the
   internal storage.

 (2) AbstractJrtFilesystem.java

getPathMatcher:
 
 if (syntax.equalsIgnoreCase(GLOB_SYNTAX)) {
  expr = JrtUtils.toRegexPattern(input);
  } else if (syntax.equalsIgnoreCase(REGEX_SYNTAX)) {
   expr = input;
  } else {
  throw new UnsupportedOperationException("Syntax '" +
 syntax
  + "' not recognized");
 }

 (3) can't JrtFileSystem use sun.nio.fs.Globs ? instead of its own
 copy?

 (4) JrtFilesystem.nodesToIterator()

  Stream has a "iterator() method. why need a collect() here?
 different performance?
  for example

private Iterator nodesToIterator(AbstractJrtPath dir,
 List childNodes) {
  return childNodes.stream()
   .map(child ->
 (Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
   .iterator();
}


 sherman


>

jlink tool review (Re: Initial webrev with changes for JDK 9)

[Michael Haupt]

Alan, all,

please find a patch with suggested changes to jlink in the attachment. The 
patch also contains a file named review-comments.txt, which addresses several 
topics throughout jlink. I've covered most code; only the jlink.internal 
package is still missing. I've been able to compile jake with these 
refactorings applied; I have not yet run all the jlink tests.

Unfortunately, I have to lay down the review work at this time; Sundar is 
taking over (thanks!!). I'm available for clarification matters.

Best,

Michael



-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: jlink tool review (Re: Initial webrev with changes for JDK 9)

[Michael Haupt]

Hi again,

some certain list server doesn't like attachments. ;-)
Find it at http://cr.openjdk.java.net/~mhaupt/jigsaw/

Best,

Michael

> Am 14.03.2016 um 13:49 schrieb Michael Haupt :
> 
> Alan, all,
> 
> please find a patch with suggested changes to jlink in the attachment. The 
> patch also contains a file named review-comments.txt, which addresses several 
> topics throughout jlink. I've covered most code; only the jlink.internal 
> package is still missing. I've been able to compile jake with these 
> refactorings applied; I have not yet run all the jlink tests.
> 
> Unfortunately, I have to lay down the review work at this time; Sundar is 
> taking over (thanks!!). I'm available for clarification matters.
> 
> Best,
> 
> Michael


-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: jlink tool review (Re: Initial webrev with changes for JDK 9)

[Sundararajan Athijegannathan]

Hi,

Thanks for the review. I've filed a bug to track your suggestions:
https://bugs.openjdk.java.net/browse/JDK-8151896

Thanks,
-Sundar

On 3/14/2016 6:26 PM, Michael Haupt wrote:
> Hi again,
>
> some certain list server doesn't like attachments. ;-)
> Find it at http://cr.openjdk.java.net/~mhaupt/jigsaw/
>
> Best,
>
> Michael
>
>> Am 14.03.2016 um 13:49 schrieb Michael Haupt :
>>
>> Alan, all,
>>
>> please find a patch with suggested changes to jlink in the attachment. The 
>> patch also contains a file named review-comments.txt, which addresses 
>> several topics throughout jlink. I've covered most code; only the 
>> jlink.internal package is still missing. I've been able to compile jake with 
>> these refactorings applied; I have not yet run all the jlink tests.
>>
>> Unfortunately, I have to lay down the review work at this time; Sundar is 
>> taking over (thanks!!). I'm available for clarification matters.
>>
>> Best,
>>
>> Michael
>