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 changes to class loader hierarchy and no changes to
visibility except that some non-core modules are no longer defined to
the boot loader.

In java.lang then Class and ClassLoader have several updates and new
methods to support modules. The legacy Package API and javadoc has also
been overhauled. The System class has been updated to support the
initialization of the module system.

Core reflection has been updated so that access control is aligned with
the Java Language and VM, except that it assumes readability (a
discussion point in the JSR at this time). Proxy has been significantly
updated so that the package, module and accessibility of the generated
proxy classes are in line with the accessibility of the proxy interfaces.

The MethodHandle API has also been updated but I expect there will be
changes soon on this, maybe additional lookup modes and changes when
teleporting from one module to another (as things stand then all access
is lost).

The ServiceLoader API has been updated to support instantiating service
providers that are deployed as modules. It also has new support for
iterating over service providers in Layers.

The ResourceBundle API has been significantly updated to support
resources deployed as modules. There are also a lot of changes to the
locale providers.

This repository has the jlink tool (JEP 282) to assembly a set of
modules as a modular run-time image (JEP 220). The jlink tool is invoked
in the JDK build to create the JDK and JRE run-time images as I
mentioned above.

The jlink tool includes an experimental API for developing plugins that
do transformations or optimizations at link-time. There are currently 11
plugins in the webrev, the most significant is the "installed-modules"
plugin that generates code at link-time to speed up the reconstitution
of module descriptors during startup.

There are significant updates to the jimage container implementation and
jrtfs. The most obvious that is there is now only one jimage container
(named "modules") in the run-time image.

The java launcher has been updated to support module paths and the other
command line options described in JEP 261.

The jar tool has been updated to support modules packaged as modular JAR
files. It has also been refurbished with support for GNU style command
line options.

JDI and JDWP have been updated to allow debugger enumerate and
introspect modules in the target VM. java.lang.instrument has initial
updates to support agents that instrument code in modules.

There are smaller changes in many others including updates to the
logging API, the JMX implementation, Image I/O, JNDI and several others.
Most of these changes are localized and should be straight-forward to
understand (esp as the code is organized by module).

There are lot of new tests. Some of the new tests aren't in the right
location yet and we'll resolve that soon. There are fewer updates to
existing tests that might be expected and this is because we've been
able to get the changes to several thousand tests into JDK 9 in advance.

Jim Laskey and Sundararajan Athijegannathan will take point for the
jlink, jimage and jrtfs changes.

For everything else then assume that Mandy Chung and I will take point
for now. We have approached many jdk9 Reviewers to help get through this.


** nashorn repository **

Nashorn has been updated to work with a modular runtime, including
spinning dynamic modules. This is the first of the dynamic languages to
get working and we'll need to learn from this to see what might
potentially need to exposed further in the API.

Sundar will take point on this, with Reviewers from the nashorn project.


** jaxp repository **

JAXP has a small update to support the XSLTC generating of translets in
modules at runtime. We need to re-visit this at some point to generate
the translets in their own layer.


** jaxws repository **

Most of the changes to JAXB and JAX-WS to work with modules are already
in JDK 9 and pushed to the upstream Metro project. There are API changes
so there will be updates to JSR 222 and JSR 224.

The residual changes in the jaxws repository are mostly handling of
resources in modules.


** corba repository **

No changes here except the module declaration.


I think that is mostly it for now. I will published new webrevs
periodically to take account of the ongoing changes.

On wider communication, then we'll send mail to jdk9-dev soon to make
everyone working on the JDK 9 project aware that we are starting to plan
the integration into JDK 9.

-Alan.

[Mandy Chung]

Post by Alan Bateman
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

[Alan Bateman]

Post by Mandy Chung
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

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

[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

Post by Alan Bateman
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.

[Mandy Chung]

Post by Naoto Sato
Hello,
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.

Post by Naoto Sato
- 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.

Post by Naoto Sato
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.

Post by Naoto Sato
- 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

Post by Naoto Sato
Naoto

Post by Alan Bateman
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.

[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 <p>. We
need to update this to enclose them in a <div> and <span> 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.

[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<String>

- 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:
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)

- StringSharingDecompressor constructor with unused argument - remove


decompress/ZipDecompressor:
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases

- decompress(): use try-with-resources for safer inflation


src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument
must be aligned

- General doubt about using assert; for example to check on malloc
allocation failure

- In production environment will exhibit as segv

[Alan Bateman]

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

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

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

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

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

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

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

Will throw InternalError instead.

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

compress()? I don’t see ImageStream used anywhere except for large buffers. As a note, dynamic ByteBuffers really should be part of the JDK.

- Comments on each method would make the intent clear

Will add comments.

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

Rewrote using lambdas to clarify the algorithm.

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

Modified.

- BADMAGIC is not used

Removed.

- readFrom could throw BufferUnderflowException - undocumented

get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException] Will add explicit check.

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

Removed.

- ImageReader(path): unused - remove

Removed.

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

Added explicit check.

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

The core libs team was concerned about the performance of class loading. The pattern used here is the one followed by NIO. We will probably come up with a shared NIO cache at some point.

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

The image cache is only used by 32 bit systems and decompression. It is possible for a channel read buffer and a decompression buffer to be allocated at the same time.

- unused create method

Removed.

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

Removed.

- constructor has unused parent arg

Removed

- readInt: wasteful allocation of a single byte array and then another small byte array;
and then another heap buffer allocation in the ByteBuffer.

Removed all allocation.

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

Removed all allocation.

- MAGIC does not need to be public
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation

I think decompression needs significant reworking..

src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument must be aligned
- General doubt about using assert; for example to check on malloc allocation failure
- In production environment will exhibit as segv

Noted.

[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

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

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

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

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

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

Will throw InternalError instead.

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

compress()? I don’t see ImageStream used anywhere except for large buffers. As a note, dynamic ByteBuffers really should be part of the JDK.

- Comments on each method would make the intent clear

Will add comments.

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

Rewrote using lambdas to clarify the algorithm.

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

Modified.

- BADMAGIC is not used

Removed.

- readFrom could throw BufferUnderflowException - undocumented

get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException] Will add explicit check.

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

Removed.

- ImageReader(path): unused - remove

Removed.

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

Added explicit check.

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

The core libs team was concerned about the performance of class loading. The pattern used here is the one followed by NIO. We will probably come up with a shared NIO cache at some point.

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

The image cache is only used by 32 bit systems and decompression. It is possible for a channel read buffer and a decompression buffer to be allocated at the same time.

- unused create method

Removed.

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

Removed.

- constructor has unused parent arg

Removed

- readInt: wasteful allocation of a single byte array and then another small byte array;
and then another heap buffer allocation in the ByteBuffer.

Removed all allocation.

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

Removed all allocation.

- MAGIC does not need to be public
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation

I think decompression needs significant reworking..

src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument must be aligned
- General doubt about using assert; for example to check on malloc allocation failure
- In production environment will exhibit as segv

Noted.

[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

Post by 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

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

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

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

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

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

Will throw InternalError instead.

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

compress()? I don’t see ImageStream used anywhere except for large buffers. As a note, dynamic ByteBuffers really should be part of the JDK.

- Comments on each method would make the intent clear

Will add comments.

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

Rewrote using lambdas to clarify the algorithm.

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

Modified.

- BADMAGIC is not used

Removed.

- readFrom could throw BufferUnderflowException - undocumented

get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException] Will add explicit check.

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

Removed.

- ImageReader(path): unused - remove

Removed.

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

Added explicit check.

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

The core libs team was concerned about the performance of class loading. The pattern used here is the one followed by NIO. We will probably come up with a shared NIO cache at some point.

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

The image cache is only used by 32 bit systems and decompression. It is possible for a channel read buffer and a decompression buffer to be allocated at the same time.

- unused create method

Removed.

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

Removed.

- constructor has unused parent arg

Removed

- readInt: wasteful allocation of a single byte array and then another small byte array;
and then another heap buffer allocation in the ByteBuffer.

Removed all allocation.

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

Removed all allocation.

- MAGIC does not need to be public
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation

I think decompression needs significant reworking..

src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument must be aligned
- General doubt about using assert; for example to check on malloc allocation failure
- In production environment will exhibit as segv

Noted.

[Jim Laskey (Oracle)]

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

Post by 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

Post by 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

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

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

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

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

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

Will throw InternalError instead.

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

compress()? I don’t see ImageStream used anywhere except for large buffers. As a note, dynamic ByteBuffers really should be part of the JDK.

- Comments on each method would make the intent clear

Will add comments.

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

Rewrote using lambdas to clarify the algorithm.

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

Modified.

- BADMAGIC is not used

Removed.

- readFrom could throw BufferUnderflowException - undocumented

get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException] Will add explicit check.

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

Removed.

- ImageReader(path): unused - remove

Removed.

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

Added explicit check.

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

The core libs team was concerned about the performance of class loading. The pattern used here is the one followed by NIO. We will probably come up with a shared NIO cache at some point.

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

The image cache is only used by 32 bit systems and decompression. It is possible for a channel read buffer and a decompression buffer to be allocated at the same time.

- unused create method

Removed.

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

Removed.

- constructor has unused parent arg

Removed

- readInt: wasteful allocation of a single byte array and then another small byte array;
and then another heap buffer allocation in the ByteBuffer.

Removed all allocation.

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

Removed all allocation.

- MAGIC does not need to be public
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation

I think decompression needs significant reworking..

src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument must be aligned
- General doubt about using assert; for example to check on malloc allocation failure
- In production environment will exhibit as segv

Noted.

[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

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

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

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

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

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

Will throw InternalError instead.

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

compress()? I don’t see ImageStream used anywhere except for large buffers. As a note, dynamic ByteBuffers really should be part of the JDK.

- Comments on each method would make the intent clear

Will add comments.

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

Rewrote using lambdas to clarify the algorithm.

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

Modified.

- BADMAGIC is not used

Removed.

- readFrom could throw BufferUnderflowException - undocumented

get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException] Will add explicit check.

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

Removed.

- ImageReader(path): unused - remove

Removed.

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

Added explicit check.

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

The core libs team was concerned about the performance of class loading. The pattern used here is the one followed by NIO. We will probably come up with a shared NIO cache at some point.

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

The image cache is only used by 32 bit systems and decompression. It is possible for a channel read buffer and a decompression buffer to be allocated at the same time.

- unused create method

Removed.

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

Removed.

- constructor has unused parent arg

Removed

- readInt: wasteful allocation of a single byte array and then another small byte array;
and then another heap buffer allocation in the ByteBuffer.

Removed all allocation.

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

Removed all allocation.

- MAGIC does not need to be public
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation

I think decompression needs significant reworking..

src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument must be aligned
- General doubt about using assert; for example to check on malloc allocation failure
- In production environment will exhibit as segv

Noted.

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

Post by Alan Bateman
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 changes to class loader hierarchy and no changes to
visibility except that some non-core modules are no longer defined to
the boot loader.
In java.lang then Class and ClassLoader have several updates and new
methods to support modules. The legacy Package API and javadoc has
also been overhauled. The System class has been updated to support the
initialization of the module system.
Core reflection has been updated so that access control is aligned
with the Java Language and VM, except that it assumes readability (a
discussion point in the JSR at this time). Proxy has been
significantly updated so that the package, module and accessibility of
the generated proxy classes are in line with the accessibility of the
proxy interfaces.
The MethodHandle API has also been updated but I expect there will be
changes soon on this, maybe additional lookup modes and changes when
teleporting from one module to another (as things stand then all
access is lost).
The ServiceLoader API has been updated to support instantiating
service providers that are deployed as modules. It also has new
support for iterating over service providers in Layers.
The ResourceBundle API has been significantly updated to support
resources deployed as modules. There are also a lot of changes to the
locale providers.
This repository has the jlink tool (JEP 282) to assembly a set of
modules as a modular run-time image (JEP 220). The jlink tool is
invoked in the JDK build to create the JDK and JRE run-time images as
I mentioned above.
The jlink tool includes an experimental API for developing plugins
that do transformations or optimizations at link-time. There are
currently 11 plugins in the webrev, the most significant is the
"installed-modules" plugin that generates code at link-time to speed
up the reconstitution of module descriptors during startup.
There are significant updates to the jimage container implementation
and jrtfs. The most obvious that is there is now only one jimage
container (named "modules") in the run-time image.
The java launcher has been updated to support module paths and the
other command line options described in JEP 261.
The jar tool has been updated to support modules packaged as modular
JAR files. It has also been refurbished with support for GNU style
command line options.
JDI and JDWP have been updated to allow debugger enumerate and
introspect modules in the target VM. java.lang.instrument has initial
updates to support agents that instrument code in modules.
There are smaller changes in many others including updates to the
logging API, the JMX implementation, Image I/O, JNDI and several
others. Most of these changes are localized and should be
straight-forward to understand (esp as the code is organized by module).
There are lot of new tests. Some of the new tests aren't in the right
location yet and we'll resolve that soon. There are fewer updates to
existing tests that might be expected and this is because we've been
able to get the changes to several thousand tests into JDK 9 in advance.
Jim Laskey and Sundararajan Athijegannathan will take point for the
jlink, jimage and jrtfs changes.
For everything else then assume that Mandy Chung and I will take point
for now. We have approached many jdk9 Reviewers to help get through this.
** nashorn repository **
Nashorn has been updated to work with a modular runtime, including
spinning dynamic modules. This is the first of the dynamic languages
to get working and we'll need to learn from this to see what might
potentially need to exposed further in the API.
Sundar will take point on this, with Reviewers from the nashorn project.
** jaxp repository **
JAXP has a small update to support the XSLTC generating of translets
in modules at runtime. We need to re-visit this at some point to
generate the translets in their own layer.
** jaxws repository **
Most of the changes to JAXB and JAX-WS to work with modules are
already in JDK 9 and pushed to the upstream Metro project. There are
API changes so there will be updates to JSR 222 and JSR 224.
The residual changes in the jaxws repository are mostly handling of
resources in modules.
** corba repository **
No changes here except the module declaration.
I think that is mostly it for now. I will published new webrevs
periodically to take account of the ongoing changes.
On wider communication, then we'll send mail to jdk9-dev soon to make
everyone working on the JDK 9 project aware that we are starting to
plan the integration into JDK 9.
-Alan.

[Alan Bateman]

Post by 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/

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

[Seán Coffey]

Post by Alan Bateman

Post by 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/

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.

Post by Alan Bateman
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.

Post by Alan Bateman
-Alan

[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

[Paul Sandoz]

Post by Alan Bateman
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<Void> 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 a volatile field in this object. Returns
2576 * {@code true} if not beaten by another thread. Avoids the use of
2577 * AtomicReferenceFieldUpdater in this class.
2578 */
2579 private boolean trySetObjectField(String name, Object obj) {
2580 Unsafe unsafe = Unsafe.getUnsafe();
2581 Class<?> k = ClassLoader.class;
2582 long offset;
2583 try {
2584 Field f = k.getDeclaredField(name);
2585 offset = unsafe.objectFieldOffset(f);
2586 } catch (NoSuchFieldException e) {
2587 throw new InternalError(e);
2588 }
2589 return unsafe.compareAndSwapObject(this, offset, null, obj);
2590 }

If you can syncrhonize on this, just use double checked locking?



j.l.i.BoundMethodHandle
—

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

That’s odd. Why did that need commenting out?


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.


j.l.r.Proxy
—

636 private static final WeakCache<Module, List<Class<?>>, Class<?>> proxyCache =
637 new WeakCache<>(new KeyFactory<Module>(),
638 new BiFunction<Module, List<Class<?>>, Class<?>>() {
639 @Override
640 public Class<?> apply(Module m, List<Class<?>> 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.


781 static Set<Class<?>> referencedTypes(ClassLoader loader, List<Class<?>> interfaces) {
782 Set<Class<?>> types = new HashSet<>();
783 for (Class<?> intf : interfaces) {
784 for (Method m : intf.getMethods()) {
785 // return type, parameter types, and exception types
786 Stream.concat(Stream.concat(Stream.of(m.getReturnType()),
787 Arrays.stream(m.getParameterTypes())),
788 Arrays.stream(m.getExceptionTypes()))
789 .map(ProxyBuilder::getElementType)
790 .filter(t -> !t.isPrimitive())
791 .forEach(types::add);
792 }
793 }
794 return types;
795 }

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<Class<?>> f(Method m) {
return Stream.of(new Class[] {m.getReturnType()}, m.getParameterTypes(), m.getExceptionTypes()).
flatMap(a -> Stream.of(a));
}


939 private static final WeakHashMap<ClassLoader, Module> dynProxyModules = new WeakHashMap<>();
940 private static final AtomicInteger counter = new AtomicInteger();
941
942 /*
943 * Define a dynamic module for the generated proxy classes in a non-exported package
944 * named com.sun.proxy.$MODULE.
945 *
946 * Each class loader will have one dynamic module.
947 */
948 static synchronized Module getDynamicModule(ClassLoader loader) {
949 Module m = dynProxyModules.get(loader);
950 if (m == null) {
951 String mn = "jdk.proxy" + counter.incrementAndGet();
952 String pn = PROXY_PACKAGE_PREFIX + "." + mn;
953 m = Modules.defineModule(loader, mn, Collections.singleton(pn));
954 Modules.addReads(m, Proxy.class.getModule());
955 // java.base to create proxy instance
956 Modules.addExports(m, pn, Object.class.getModule());
957 dynProxyModules.put(loader, m);
958 }
959 return m;
960 }

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


1055 @CallerSensitive
1056 public static Object newProxyInstance(ClassLoader loader,
1057 Class<?>[] interfaces,
1058 InvocationHandler h) {
1059 Objects.requireNonNull(h);
1060
1061 final List<Class<?>> intfs = Arrays.asList(interfaces.clone());

Use List.of, which will also clone? This presumes that there are no null values in the array.


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…,


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

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

reader -> READER


sun.util.resources.LocaleData
—

77 private static final Map<String, List<Locale>> candidatesMap = new ConcurrentHashMap<>();

candidatesMap -> CANDIDATES_MAP


native/libjimage/imageDecompressor.cpp
—

114 // Sparc to read unaligned content
115 // u8 l = (*(u8*) ptr);
116 // If ptr is not aligned, sparc will fail.
117 u8 ImageDecompressor::getU8(u1* ptr, Endian *endian) {
118 u8 ret;
119 if (endian->is_big_endian()) {
120 ret = (u8)ptr[0] << 56 | (u8)ptr[1] << 48 | (u8)ptr[2]<<40 | (u8)ptr[3]<<32 |
121 ptr[4]<<24 | ptr[5]<<16 | ptr[6]<<8 | ptr[7];
122 } else {
123 ret = ptr[0] | ptr[1]<<8 | ptr[2]<<16 | ptr[3]<<24 | (u8)ptr[4]<<32 |
124 (u8)ptr[5]<<40 | (u8)ptr[6]<<48 | (u8)ptr[7]<<56;
125 }
126 return ret;
127 }
128
129 u4 ImageDecompressor::getU4(u1* ptr, Endian *endian) {
130 u4 ret;
131 if (endian->is_big_endian()) {
132 ret = ptr[0] << 24 | ptr[1]<<16 | (ptr[2]<<8) | ptr[3];
133 } else {
134 ret = ptr[0] | ptr[1]<<8 | (ptr[2]<<16) | ptr[3]<<24;
135 }
136 return ret;
137 }


I dunno if hotspot/src/share/vm/utilities/bytes.hpp can be leveraged here.


java.lang.module.ModuleInfo
—

460 private static boolean isAttributeDisallowed(String name) {
461 Set<String> notAllowed = predefinedNotAllowed;
462 if (notAllowed == null) {
463 notAllowed = Stream.of(
464 "ConstantValue",
465 "Code",
466 "StackMapTable",
467 "Exceptions",
468 "EnclosingMethod",
469 "Signature",
470 "LineNumberTable",
471 "LocalVariableTable",
472 "LocalVariableTypeTable",
473 "RuntimeVisibleAnnotations",
474 "RuntimeInvisibleAnnotations",
475 "RuntimeVisibleParameterAnnotations",
476 "RuntimeInvisibleParameterAnnotations",
477 "RuntimeVisibleTypeAnnotations",
478 "RuntimeInvisibleTypeAnnotations",
479 "AnnotationDefault",
480 "BootstrapMethods",
481 "MethodParameters")
482 .collect(Collectors.toSet());
483 predefinedNotAllowed = notAllowed;

Consider using Set.of


484 }
485
486 if (notAllowed.contains(name)) {
487 return true;
488 } else {
489 return false;
490 }
491 }


j.l.ModuleReader
—

145 default Optional<ByteBuffer> read(String name) throws IOException {
146 final int BUFFER_SIZE = 8192;
147 final int MAX_BUFFER_SIZE = Integer.MAX_VALUE - 8;
148
149 InputStream in = open(name).orElse(null);
150 if (in == null) {
151 // not found
152 return Optional.empty();
153 }
154
155 try (in) {
156 int capacity = in.available();
157 if (capacity == 0)
158 capacity = BUFFER_SIZE;
159
160 byte[] buf = new byte[capacity];
161 int nread = 0;
162 int n;
163 for (;;) {

Use InputStream.readAllBytes?


jdk.internal.module.Builder
—

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

Set.of ?

[Alan Bateman]

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
sun.net.www.protocol.jrt.JavaRuntimeURLConnection
—
56 private static final ImageReader reader = ImageReaderFactory.getImageReader();
reader -> READER

okay.

Post by Paul Sandoz
java.lang.module.ModuleInfo
—
Consider using Set.of

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

Post by Paul Sandoz
j.l.ModuleReader
—
Use InputStream.readAllBytes?

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

Post by Paul Sandoz
jdk.internal.module.Builder
—
56 private static final Set<Requires.Modifier> MANDATED =
57 Collections.singleton(Requires.Modifier.MANDATED);
58 private static final Set<Requires.Modifier> PUBLIC =
59 Collections.singleton(Requires.Modifier.PUBLIC);
Set.of ?

Okay.

[Paul Sandoz]

Post by Paul Sandoz
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.

Post by Paul Sandoz
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).

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
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.

[Mandy Chung]

Thanks for the feedback.

Post by Paul Sandoz
sun/util/locale/provider/ResourceBundleProviderSupport.java
—
72 // java.base may not be able to read the bundleClass's module.
73 PrivilegedAction<Void> 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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
j.l.r.Proxy
—
636 private static final WeakCache<Module, List<Class<?>>, Class<?>> proxyCache =
637 new WeakCache<>(new KeyFactory<Module>(),
638 new BiFunction<Module, List<Class<?>>, Class<?>>() {
640 public Class<?> apply(Module m, List<Class<?>> 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.

Post by Paul Sandoz
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<Class<?>> f(Method m) {
return Stream.of(new Class[] {m.getReturnType()}, m.getParameterTypes(), m.getExceptionTypes()).
flatMap(a -> Stream.of(a));
}

Updated.

Post by Paul Sandoz
Consider using computeIfAbsent. At the moment no need for AtomicInteger.

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

Post by Paul Sandoz
1056 public static Object newProxyInstance(ClassLoader loader,
1057 Class<?>[] interfaces,
1058 InvocationHandler h) {
1059 Objects.requireNonNull(h);
1060
1061 final List<Class<?>> 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.

Post by Paul Sandoz
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.

Post by Paul Sandoz
sun.util.resources.LocaleData
—
77 private static final Map<String, List<Locale>> 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] http://download.java.net/java/jigsaw/docs/api/java/lang/reflect/Proxy.html#membership

[Paul Sandoz]

Post by Mandy Chung
Thanks for the feedback.

Post by Paul Sandoz
sun/util/locale/provider/ResourceBundleProviderSupport.java
—
72 // java.base may not be able to read the bundleClass's module.
73 PrivilegedAction<Void> 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!

Post by Mandy Chung

Post by Paul Sandoz
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.

Post by Mandy Chung

Post by Paul Sandoz
j.l.r.Proxy
—
636 private static final WeakCache<Module, List<Class<?>>, Class<?>> proxyCache =
637 new WeakCache<>(new KeyFactory<Module>(),
638 new BiFunction<Module, List<Class<?>>, Class<?>>() {
640 public Class<?> apply(Module m, List<Class<?>> 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.

Post by Mandy Chung

Post by Paul Sandoz
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.

[Mandy Chung]

Post by Paul Sandoz

Post by Mandy Chung
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.

Post by Paul Sandoz
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

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

Post by Paul Sandoz
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

[Peter Levart]

Sorry,

Post by Peter Levart
If you also care for constant lambda, this could be optimized even
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) -> {

Post by Peter Levart
assert p != null;
// replace NamedPackage with Package
NamedPackage.toPackage(p.name(), p.module());
});

[Mandy Chung]

Hi Peter,

Thanks for the review feedback.

Post by Peter Levart
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.
Simple clean code as above is good for this case.

Post by Peter Levart
Stream<Package> packages() {
return packages.values().stream()
.map(p -> definePackage(p.packageName(), p.module()));
}
Stream<Package> 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

[Mandy Chung]

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

Stream<Package> packages() {
return packages.values().stream()
.map(p -> definePackage(p.packageName(), p.module()));
}
Stream<Package> 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

[Peter Levart]

Hi Mandy,

Post by Mandy Chung
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<Package> 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

[Mandy Chung]

Post by Peter Levart
Hi Mandy,

Post by Mandy Chung
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

[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

Post by Alan Bateman
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

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

Post by Alan Bateman
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

[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

[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<Path> nodesToIterator(AbstractJrtPath dir, List<Node> childNodes) {
return childNodes.stream()
.map(child -> (Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
.iterator();
}


sherman

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

Post by Xueming Shen
(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
internal storage.
(2) AbstractJrtFilesystem.java
....
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<Path> nodesToIterator(AbstractJrtPath dir, List<Node> childNodes) {
return childNodes.stream()
.map(child -> (Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
.iterator();
}
sherman

[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

Post by 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".

Post by Xueming Shen
(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
internal storage.
(2) AbstractJrtFilesystem.java
....
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<Path> nodesToIterator(AbstractJrtPath dir,
List<Node> childNodes) {
return childNodes.stream()
.map(child ->
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
.iterator();
}
sherman

[Xueming Shen]

Post by 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
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

Post by Sundararajan Athijegannathan
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

Post by 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".

Post by Xueming Shen
(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
internal storage.
(2) AbstractJrtFilesystem.java
....
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<Path> nodesToIterator(AbstractJrtPath dir,
List<Node> childNodes) {
return childNodes.stream()
.map(child ->
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
.iterator();
}
sherman

[Sundararajan Athijegannathan]

Post by Xueming Shen

Post by 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
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

Post by Xueming Shen
-Sherman

Post by Sundararajan Athijegannathan
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

Post by 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".

Post by Xueming Shen
(1) JrtFilePath: it does not seem to help performance to use the byte[] as the
internal storage.
(2) AbstractJrtFilesystem.java
....
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<Path> nodesToIterator(AbstractJrtPath dir,
List<Node> childNodes) {
return childNodes.stream()
.map(child ->
(Path)dir.resolve(toJrtPath(child.getNameString()).getFileName()))
.iterator();
}
sherman

[Chris Hegarty]

Thank you for the feedback Sherman.

Post by Xueming Shen
jar.Main: comments
since what we really need here is the byte[], maybe just go straightforward
to use InputStream/Files.(path)readAllBytes() ?

That is cleaner. Done.

Post by Xueming Shen
(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 .

Post by Xueming Shen
(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.

Post by Xueming Shen
(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

Post by Xueming Shen
"matcher" can be reused as
Matcher m = dependenciesToHash.matcher("");
for (...) {
m.reset(...).find() ...
}

Thanks, Done.

Post by Xueming Shen
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.

[Alan Bateman]

Post by Phil Race
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.

Post by Phil Race
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.

Post by Phil Race
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.

Post by Phil Race
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.

Post by Phil Race
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.

[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

[Yuri Nesterenko]

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

Post by Alan Bateman
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.

Post by Alan Bateman
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.

Post by Alan Bateman
test/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh
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

[Mandy Chung]

Post by Alan Bateman
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:<MODULE> 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

[Chris Hegarty]

Mandy,

Post by Mandy Chung

Post by Alan Bateman
http://cr.openjdk.java.net/~alanb/8142968/2

299 private boolean printModuleDescriptor(InputStream in)
jmod -p option prints the output in different sections.
java -listmods:<MODULE> 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:<MODULE>.

Post by Mandy Chung
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.

Post by Mandy Chung
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.

Post by Mandy Chung
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.

Post by Mandy Chung
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.

[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

Post by Alan Bateman
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

[Mandy Chung]

Daniel,

Thanks for the review.

Post by Daniel Fuchs
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 }
return Objects.hash(this.filename, this.path);

Good idea. I made the change.

Post by Daniel Fuchs
443 // otherwise analyze the depednencies

Fixed.

Mandy

[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
--
<http://www.oracle.com/>
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: [phone] | Fax: [phone]
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
<http://www.oracle.com/commitment> Oracle is committed to developing practices and products that help protect the environment

[Michael Haupt]

Hi again,

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

Best,

Michael

Post by 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

--
<http://www.oracle.com/>
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: [phone] | Fax: [phone]
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
<http://www.oracle.com/commitment> Oracle is committed to developing practices and products that help protect the environment

[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

Post by Michael Haupt
Hi again,
some certain list server doesn't like attachments. ;-)
Find it at http://cr.openjdk.java.net/~mhaupt/jigsaw/
Best,
Michael

Post by 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

[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

[Peter Levart]

Hi Alan,

Just one nit.

Post by Alan Bateman
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<Package> of defined Packages which unnecessarily
calls definePackage() for each value of packages map:

Stream<Package> 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<Package> packages() {
return packages.values().stream()
.map(p -> p instanceof Package
? (Package) p
: definePackage(p.packageName(),
p.module()));
}


Regards, Peter

[Peter Levart]

Hi Alan,

Post by Alan Bateman
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

[Alan Bateman]

Post by Peter Levart
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
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
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

[Mandy Chung]

Post by Peter Levart
Hi Alan,

Post by Alan Bateman
http://cr.openjdk.java.net/~alanb/8142968/3

I have another optimization...
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.
http://cr.openjdk.java.net/~plevart/jake/Proxy.addPackage.opt/webrev.01/

I’ll sponsor this patch.

Mandy