Re: 8154956: Module system implementation refresh (4/2016)
Hi Peter,
src/java.base/share/classes/java/lang/module/ModuleReader.java
146 if (oin.isPresent()) try (InputStream in = oin.get()) {
147 return Optional.of(ByteBuffer.wrap(in.readAllBytes()));
148 } else {
149 return Optional.empty();
150 }
I highly doubt that the line 146 is easy to read. Can you change it into:
146 if (oin.isPresent()) {
147 try (InputStream in = oin.get()) {
148 return Optional.of(ByteBuffer.wrap(in.readAllBytes()));
149 }
150 }
151 return Optional.empty();
And maybe you can add following changes to your patch too.
http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.02/src/java.base/share/classes/java/lang/module/ModulePath.java.html
The following lines use the String-based version of the #lastIndexOf-method.
341 int index = cf.lastIndexOf("/") + 1;
391 int i = fn.lastIndexOf(File.separator);
568 int index = cn.lastIndexOf("/");
They should be changed to use the char-based version.
I don't know how often the method #deriveModuleDescriptor(JarFile)
is executed. But compiling regular expressions over and over doesn't really
helps from the performance standpoint of view:
400 Matcher matcher = Pattern.compile("-(\\d+(\\.|$))").matcher(mn);
415 mn = mn.replaceAll("[^A-Za-z0-9]", ".") // replace
non-alphanumeric
416 .replaceAll("(\\.)(\\1)+", ".") // collapse
repeating dots
417 .replaceAll("^\\.", "") // drop leading dots
418 .replaceAll("\\.$", ""); // drop trailing dots
Maybe the regular expressions in the above lines should be precompiled
in static final fields (Pattern objects are immutable and thread safe)
to improve
the performance of the #deriveModuleDescriptor(JarFile)-method.
Best regards,
Andrej Golovnin
Re: 8154956: Module system implementation refresh (4/2016)
Hi,
Last minute findings...
In ModulePath, jarPackages() and jmodPackages() helper method both use
toPackageName(ZipEntry entry) method which checks for "classes/" prefix
and strips it away. But "classes" is a valid package name or prefix (in
case it is found in a .jar file). So it's better to only strip it off
for jmodPackages()...
http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.02/
Regards, Peter
On 05/02/2016 12:27 AM, Peter Levart wrote:
Hi,
I have some mostly stylish changes (which you can use or not)
regarding usage of Optional and Stream(s) in package java.lang.module:
http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/
...with some optimizations/cleanups/fixes:
ModuleFinder.compose:
- the composed ModuleFinder.findAll() is more efficient this way as it
doesn't need to call back to find().
ModulePath:
- there's unneeded overhead to call distinct() for a Stream that is
later collected into Collectors.toSet() as the collector already
uniquifies the elements.
- lines 432...437 (old) - you don't want to treat entries in
SERVICES_PREFIX directory that end with ".class" as classFiles - so I
changes the:
.filter(e -> (e.endsWith(".class") ||
e.startsWith(SERVICES_PREFIX)))
to:
.filter(s -> (s.endsWith(".class") ^
s.startsWith(SERVICES_PREFIX)))
ModuleReader:
- the default method read(String name) should close the InputStream
after reading from it, shouldn't it?
ModuleReferences:
- no need for 'closed' flag in SafeCloseModuleReader to be volatile as
it is always accessed under lock (either read lock when reading or
write lock when writing).
Resolver:
- Implemented caching of ResolvedModule(s) as they are entered into
resolved graph so that there are no duplicates.
- Simplified last stage of iterative algorithm to build resolved
graph. There's no need to construct a map of changes and apply it
afterwards to 'g1' as changes can be applied individually for each
module 'm1' in the graph 'g1' as they are collected.
Regards, Peter
On 04/29/2016 02:38 PM, Alan Bateman wrote:
There have several changes in jake that we need to bring into
jdk9/dev. The main changes are:
1. The policy described in JEP 261 for the root modules to resolve
when compiling code in the unnamed module or where the main class is
loaded from the class file. This is a disruptive change and we need
to get through the transition.
Related is a new token `ALL-DEFAULT` that can be specified to
`-addmods` to resolve the same roots when the initial module is a
named module. This will eventually replace `ALL-SYSTEM` but we can't
remove that just yet. The launchers for javac, javadoc, jlink and a
few other tools that load arbitrary code in unnamed modules are now
compiled with this option.
2. The transition to the new form of -Xpatch. This is mostly changes
in hotspot but there are changes in other repos too to drop or
replace the old form of -Xpatch (boot cycle builds for example).
3. Removal of the old form of -XaddExports and -XaddReads. This has
build + test changes.
4. The second phase of integrity hashing. With this phase then the
build records in java.base the hashes of the non-upgradeable modules.
This prevents accidental linking of standard/JDK modules from
different JDK builds. The jar and jmod tools have updated options to
support this.
5. Peter Levart's patch to replace the internal WeakSet in jlr.Module
with a WeakKeyPair.
6. Updates to jlink option handling. The reason this went into
jdk9/dev is that it is disruptive to FX packager and it's hard to
coordinate with FX when it's a separate forest. The packager class
that Chris Bensen has added to jlink will allows us to iterate on
jlink with reduced risk of breaking FX.
There are several small bug fixes and clean-ups in several areas,
we'll have a lot more of these for the next update.
One other thing to mention is that we've bumped the required version
of jtreg as jtreg relies on-Xpatch to add test cases into modules and
so it needed to be updated too.
The webrevs, all repos are here:
http://cr.openjdk.java.net/~alanb/8154956/1/
There are a couple of files in the webrevs that we probably won't
bring to JDK 9 in this update:
i. We have a patch to IDL compiler in the corba repo that needs a
more extensive fix.
ii. The javadoc change in ModuleFinder as there are still details to
decide on how modular JARs work as multi-release JARs.
One other point is that the webrevs are against jdk-9+116 for now.
I've done a test merge + build with the current jdk9/dev forest and
there are only a few conflicts to fix. I will re-merge + test with
jdk9/dev once we have agreed the changes for this update.
Finally, just to say that we'll probably continue in jake for a while
after we get through this update. There are several design issues on
the JSR issues list that will likely require a few iterations and a
bit of bake
Re: 8154956: Module system implementation refresh (4/2016)
Hi,
I have some mostly stylish changes (which you can use or not) regarding
usage of Optional and Stream(s) in package java.lang.module:
http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/
...with some optimizations/cleanups/fixes:
ModuleFinder.compose:
- the composed ModuleFinder.findAll() is more efficient this way as it
doesn't need to call back to find().
ModulePath:
- there's unneeded overhead to call distinct() for a Stream that is
later collected into Collectors.toSet() as the collector already
uniquifies the elements.
- lines 432...437 (old) - you don't want to treat entries in
SERVICES_PREFIX directory that end with ".class" as classFiles - so I
changes the:
.filter(e -> (e.endsWith(".class") ||
e.startsWith(SERVICES_PREFIX)))
to:
.filter(s -> (s.endsWith(".class") ^
s.startsWith(SERVICES_PREFIX)))
ModuleReader:
- the default method read(String name) should close the InputStream
after reading from it, shouldn't it?
ModuleReferences:
- no need for 'closed' flag in SafeCloseModuleReader to be volatile as
it is always accessed under lock (either read lock when reading or write
lock when writing).
Resolver:
- Implemented caching of ResolvedModule(s) as they are entered into
resolved graph so that there are no duplicates.
- Simplified last stage of iterative algorithm to build resolved graph.
There's no need to construct a map of changes and apply it afterwards to
'g1' as changes can be applied individually for each module 'm1' in the
graph 'g1' as they are collected.
Regards, Peter
On 04/29/2016 02:38 PM, Alan Bateman wrote:
There have several changes in jake that we need to bring into
jdk9/dev. The main changes are:
1. The policy described in JEP 261 for the root modules to resolve
when compiling code in the unnamed module or where the main class is
loaded from the class file. This is a disruptive change and we need to
get through the transition.
Related is a new token `ALL-DEFAULT` that can be specified to
`-addmods` to resolve the same roots when the initial module is a
named module. This will eventually replace `ALL-SYSTEM` but we can't
remove that just yet. The launchers for javac, javadoc, jlink and a
few other tools that load arbitrary code in unnamed modules are now
compiled with this option.
2. The transition to the new form of -Xpatch. This is mostly changes
in hotspot but there are changes in other repos too to drop or replace
the old form of -Xpatch (boot cycle builds for example).
3. Removal of the old form of -XaddExports and -XaddReads. This has
build + test changes.
4. The second phase of integrity hashing. With this phase then the
build records in java.base the hashes of the non-upgradeable modules.
This prevents accidental linking of standard/JDK modules from
different JDK builds. The jar and jmod tools have updated options to
support this.
5. Peter Levart's patch to replace the internal WeakSet in jlr.Module
with a WeakKeyPair.
6. Updates to jlink option handling. The reason this went into
jdk9/dev is that it is disruptive to FX packager and it's hard to
coordinate with FX when it's a separate forest. The packager class
that Chris Bensen has added to jlink will allows us to iterate on
jlink with reduced risk of breaking FX.
There are several small bug fixes and clean-ups in several areas,
we'll have a lot more of these for the next update.
One other thing to mention is that we've bumped the required version
of jtreg as jtreg relies on-Xpatch to add test cases into modules and
so it needed to be updated too.
The webrevs, all repos are here:
http://cr.openjdk.java.net/~alanb/8154956/1/
There are a couple of files in the webrevs that we probably won't
bring to JDK 9 in this update:
i. We have a patch to IDL compiler in the corba repo that needs a more
extensive fix.
ii. The javadoc change in ModuleFinder as there are still details to
decide on how modular JARs work as multi-release JARs.
One other point is that the webrevs are against jdk-9+116 for now.
I've done a test merge + build with the current jdk9/dev forest and
there are only a few conflicts to fix. I will re-merge + test with
jdk9/dev once we have agreed the changes for this update.
Finally, just to say that we'll probably continue in jake for a while
after we get through this update. There are several design issues on
the JSR issues list that will likely require a few iterations and a
bit of bake time before we bring them to JDK 9.
-Alan
Re: Speeding up jmod
Hi Chris, Alan,
I have updated my patch. Additionally to the other changes
it warps the InputStream returned by Files.newInputStream()
in a BufferedInputStream in the method
JmodFileWriter#processSection(ZipOutputStream, Section, Path)
because Files.newInputStream() returns an unbuffered InputStream
per specification.
Best regards,
Andrej Golovnin
diff -r 7210b5dbd92f src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
--- a/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Sat Apr 30
16:41:08 2016 -0700
+++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Sun May 01
19:09:04 2016 +0200
@@ -373,7 +373,6 @@
final String osArch = options.osArch;
final String osVersion = options.osVersion;
final List excludes = options.excludes;
-final Hasher hasher = hasher();
JmodFileWriter() { }
@@ -400,31 +399,34 @@
* on the class path of directories and JAR files.
*/
Supplier newModuleInfoSupplier() throws IOException {
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
+byte[] bytes = null;
for (Path e: classpath) {
if (Files.isDirectory(e)) {
Path mi = e.resolve(MODULE_INFO);
if (Files.isRegularFile(mi)) {
-Files.copy(mi, baos);
+bytes = Files.readAllBytes(mi);
break;
}
} else if (Files.isRegularFile(e) &&
e.toString().endsWith(".jar")) {
try (JarFile jf = new JarFile(e.toFile())) {
ZipEntry entry = jf.getEntry(MODULE_INFO);
if (entry != null) {
-jf.getInputStream(entry).transferTo(baos);
-break;
+byte[] tmp = new byte[(int) entry.getSize()];
+if (jf.getInputStream(entry).read(tmp) ==
tmp.length) {
+bytes = tmp;
+break;
+}
}
} catch (ZipException x) {
// Skip. Do nothing. No packages will be added.
}
}
}
-if (baos.size() == 0) {
+if (bytes == null) {
return null;
} else {
-byte[] bytes = baos.toByteArray();
-return () -> new ByteArrayInputStream(bytes);
+byte[] tmp = bytes;
+return () -> new ByteArrayInputStream(tmp);
}
}
@@ -450,7 +452,6 @@
try (InputStream in = miSupplier.get()) {
descriptor = ModuleDescriptor.read(in);
}
-
// copy the module-info.class into the jmod with the additional
// attributes for the version, main class and other meta data
try (InputStream in = miSupplier.get()) {
@@ -479,6 +480,7 @@
if (moduleVersion != null)
extender.version(moduleVersion);
+Hasher hasher = hasher(descriptor);
if (hasher != null) {
ModuleHashes moduleHashes =
hasher.computeHashes(descriptor.name());
if (moduleHashes != null) {
@@ -504,50 +506,36 @@
* The jmod file is being created and does not exist in the
* given modulepath.
*/
-private Hasher hasher() {
+private Hasher hasher(ModuleDescriptor descriptor) {
if (options.modulesToHash == null)
return null;
-try {
-Supplier miSupplier = newModuleInfoSupplier();
-if (miSupplier == null) {
-throw new IOException(MODULE_INFO + " not found");
+URI uri = options.jmodFile.toUri();
+ModuleReference mref = new ModuleReference(descriptor, uri, new
Supplier<>() {
+@Override
+public ModuleReader get() {
+throw new UnsupportedOperationException();
}
+});
-ModuleDescriptor descriptor;
-try (InputStream in = miSupplier.get()) {
-descriptor = ModuleDescriptor.read(in);
-}
+// compose a module finder with the module path and also
+// a module finder that can find the jmod file being created
+ModuleFinder finder = ModuleFinder.compose(options.moduleFinder,
+new ModuleFinder() {
+@Override
+public Optional find(String name) {
+if (descriptor.name().equals(name))
+return Optional.of(mref);
+else return Optional.empty();
+}
-URI uri =
hg: jigsaw/jake/jdk: 2 new changesets
Changeset: 2beef7ebb9f2 Author:alanb Date: 2016-05-01 16:40 +0100 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/2beef7ebb9f2 Creating JMOD should use buffered output Contributed-by: claes.redes...@oracle.com ! src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Changeset: f05d7fee4b7e Author:alanb Date: 2016-05-01 16:52 +0100 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/f05d7fee4b7e More misc. cleanup ! src/java.base/share/classes/java/lang/reflect/Module.java ! src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java ! src/java.base/share/classes/jdk/internal/module/ModuleHashes.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java ! src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModuleDescriptorPlugin.java
Re: Speeding up jmod
Hi Alan,
If you would like to improve the performance a little bit more,
you can make additional changes (see the patch) to JmodTask
and maybe Claes can rerun his tests to see if it helps.
The patch makes following changes:
- reads module info in a little bit more efficient way.
- avoids duplicate searching for module info
when hashing of modules is required.
- uses String#lastIndexOf(int) in toPackageName(ZipEntry)
instead of String#lastIndexOf(String).
- avoids checking for every directory entry in a JarFile if it ends
with “module-info.class”.
Best regards,
Andrej Golovnin
diff -r 7210b5dbd92f src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
--- a/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Sat Apr 30
16:41:08 2016 -0700
+++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Sun May 01
15:43:24 2016 +0200
@@ -373,7 +373,6 @@
final String osArch = options.osArch;
final String osVersion = options.osVersion;
final List excludes = options.excludes;
-final Hasher hasher = hasher();
JmodFileWriter() { }
@@ -400,31 +399,34 @@
* on the class path of directories and JAR files.
*/
Supplier newModuleInfoSupplier() throws IOException {
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
+byte[] bytes = null;
for (Path e: classpath) {
if (Files.isDirectory(e)) {
Path mi = e.resolve(MODULE_INFO);
if (Files.isRegularFile(mi)) {
-Files.copy(mi, baos);
+bytes = Files.readAllBytes(mi);
break;
}
} else if (Files.isRegularFile(e) &&
e.toString().endsWith(".jar")) {
try (JarFile jf = new JarFile(e.toFile())) {
ZipEntry entry = jf.getEntry(MODULE_INFO);
if (entry != null) {
-jf.getInputStream(entry).transferTo(baos);
-break;
+byte[] tmp = new byte[(int) entry.getSize()];
+if (jf.getInputStream(entry).read(tmp) ==
tmp.length) {
+bytes = tmp;
+break;
+}
}
} catch (ZipException x) {
// Skip. Do nothing. No packages will be added.
}
}
}
-if (baos.size() == 0) {
+if (bytes == null) {
return null;
} else {
-byte[] bytes = baos.toByteArray();
-return () -> new ByteArrayInputStream(bytes);
+byte[] tmp = bytes;
+return () -> new ByteArrayInputStream(tmp);
}
}
@@ -450,7 +452,6 @@
try (InputStream in = miSupplier.get()) {
descriptor = ModuleDescriptor.read(in);
}
-
// copy the module-info.class into the jmod with the additional
// attributes for the version, main class and other meta data
try (InputStream in = miSupplier.get()) {
@@ -479,6 +480,7 @@
if (moduleVersion != null)
extender.version(moduleVersion);
+Hasher hasher = hasher(descriptor);
if (hasher != null) {
ModuleHashes moduleHashes =
hasher.computeHashes(descriptor.name());
if (moduleHashes != null) {
@@ -504,50 +506,36 @@
* The jmod file is being created and does not exist in the
* given modulepath.
*/
-private Hasher hasher() {
+private Hasher hasher(ModuleDescriptor descriptor) {
if (options.modulesToHash == null)
return null;
-try {
-Supplier miSupplier = newModuleInfoSupplier();
-if (miSupplier == null) {
-throw new IOException(MODULE_INFO + " not found");
+URI uri = options.jmodFile.toUri();
+ModuleReference mref = new ModuleReference(descriptor, uri, new
Supplier<>() {
+@Override
+public ModuleReader get() {
+throw new UnsupportedOperationException();
}
+});
-ModuleDescriptor descriptor;
-try (InputStream in = miSupplier.get()) {
-descriptor = ModuleDescriptor.read(in);
-}
+// compose a module finder with the module path and also
+// a module finder that can find the jmod file being created
+ModuleFinder finder = ModuleFinder.compose(options.moduleFinder,
+new ModuleFinder() {
+@Override
+public Optional
Re: Speeding up jmod
On 01/05/2016 03:02, Claes Redestad wrote: Hi, Alan asked me to take a look at jmod performance (also jlink, but saving that for another day), so I set up a naive benchmark[1] and started profiling. ... and saw nothing really suspicious except that time is split between doing I/O and executing native code in libz.so, which I guess isn't surprising. Oddly enough the only java methods that even show up in profiles are related to writing, so I figured taking a closer look at the code for writing output from jmod wouldn't hurt. Turns out I was wrong, since I soon found that the output stream used by JmodTask is unbuffered... Applied a trivial patch[2] and results of running the micro with -f 10 -i 1 -bm ss (which is more or less like running jmod standalone): Benchmark Mode Cnt Score Error Units JmodBenchmark.jmodJavaBasess 10 1.966 ± 0.297 s/op # before JmodBenchmark.jmodJavaBasess 10 1.196 ± 0.142 s/op # after Seems like a notable reduction right there. Timing runs of jmod standalone gives analogous results on real time, but user time is still almost as high. Poking around further and it's obvious JIT threads are eating a larger portion of my cycles now - likely C2 is ramping up but not having time to get much done in the short life-time of jmod, which is mostly spent in native code anyhow. Switching to running short-running apps with only C1 can be profitable, especially on machines with a lot of cores (like the 2x8x2 machine I'm running this on), so I ran the numbers: Again, with time: Benchmark Mode Cnt Score Error Units JmodBenchmark.jmodJavaBasess 10 1.175 ± 0.147 s/op real0m17.140s user0m54.868s sys0m4.172s -XX:TieredStopAtLevel=1 BenchmarkMode Cnt Score Error Units JmodBenchmark.jmodJavaBase thrpt 10 1.075 ± 0.194 ops/s real0m14.810s user0m15.556s sys0m1.584s Yep, only running "C1" improves things a lot in this case and on my environment. I suggest accepting the patch[2] as well as switching the jmod runner to run with -XX:TieredStopAtLevel=1 or similar. Both are likely needed for most to see any effect on build times. A long term alternative to consider might be to implement a server-based jmod akin to the javac server. Thanks Claes, this is good analysis! The create method should be using a BufferedOutputStream, I'm surprised that it isn't. 'll get that patch in the current refresh although it looks like this helps more with the benchmark that with the build. I changed make/CreateJmods.java to use -XX:TieredStopAtLevel=1 and make a bit difference in the build. The wall clock time to create the jmods on my local machine drops from 46s. to 22s. I also tried a remote Windows machine and the time to create the jmods also dropped by about 20s. I'm sure Erik will have advice on how to fit this in. As things stand, the VM options for the jmod command are configured in spec.gmk.in to to use $(JAVA_TOOL_FLAGS_SMALL). Maybe it's time to change JAVA_TOOL_FLAGS_SMALL as it it seems to be -XX:+UseSerialGC and some heap settings at this time. As regards changing the jmod launcher then one concern with that it the options might conflict with options specified via -J so would need to look at that more closely. -Alan
