RFR: JDK-8147460: Clean-up jrtfs implementation
Hi, Please hep review the cleanup changes for jrtfs implementation. issue: https://bugs.openjdk.java.net/browse/JDK-8147460 webrev: http://cr.openjdk.java.net/~sherman/8147460/webrev Simple benchmark [1] has been run both on jimage and the "exploded modules" directory, the numbers suggest we also get an encouraging performance boost on most of the frequently invoked access methods. [1] http://cr.openjdk.java.net/~sherman/8147460/MyBenchmark.java Benchmark Mode Cnt Score Error Units ------------------------------------------------------------ [jimage/existing] MyBenchmark.testExists avgt 50 18.592 ± 1.213 ms/op MyBenchmark.testIsDirectory avgt 50 17.382 ± 1.178 ms/op MyBenchmark.testIsRegularFile avgt 50 18.576 ± 1.577 ms/op MyBenchmark.testItr avgt 50 58.414 ± 1.638 ms/op MyBenchmark.testLookupPath avgt 50 18.935 ± 1.093 ms/op MyBenchmark.testLookupStr avgt 50 38.597 ± 1.663 ms/op MyBenchmark.testToRealPath avgt 50 30.119 ± 1.196 ms/op [jimage/new] MyBenchmark.testExists avgt 50 10.865 ± 1.125 ms/op MyBenchmark.testIsDirectory avgt 50 11.077 ± 1.101 ms/op MyBenchmark.testIsRegularFile avgt 50 10.967 ± 1.220 ms/op MyBenchmark.testItr avgt 50 49.249 ± 3.753 ms/op MyBenchmark.testLookupPath avgt 50 10.857 ± 1.071 ms/op MyBenchmark.testLookupStr avgt 50 34.367 ± 1.341 ms/op MyBenchmark.testToRealPath avgt 50 11.460 ± 1.081 ms/op ------------------------------------------------------------ [exploded dir/existing] MyBenchmark.testExists avgt 50 23.055 ± 1.707 ms/op MyBenchmark.testIsDirectory avgt 50 23.985 ± 1.593 ms/op MyBenchmark.testIsRegularFile avgt 50 24.200 ± 1.744 ms/op MyBenchmark.testItr avgt 50 210.002 ± 6.954 ms/op MyBenchmark.testLookupPath avgt 50 23.242 ± 1.799 ms/op MyBenchmark.testLookupStr avgt 50 43.026 ± 1.751 ms/op MyBenchmark.testToRealPath avgt 50 56.536 ± 1.679 ms/op [exploded dir/new] MyBenchmark.testExists avgt 50 11.260 ± 1.209 ms/op MyBenchmark.testIsDirectory avgt 50 11.702 ± 1.069 ms/op MyBenchmark.testIsRegularFile avgt 50 12.284 ± 1.333 ms/op MyBenchmark.testItr avgt 50 49.342 ± 1.516 ms/op MyBenchmark.testLookupPath avgt 50 11.041 ± 1.051 ms/op MyBenchmark.testLookupStr avgt 50 35.850 ± 1.618 ms/op MyBenchmark.testToRealPath avgt 50 13.016 ± 1.339 ms/op Thanks, Sherman
Hi Sherman, Not a full review, I browsed the code very quickly and noticed one thing: JrtFileSystem — 167 private static final Set<String> supportedFileAttributeViews 168 = Collections.unmodifiableSet( 169 new HashSet<String>(Arrays.asList("basic", "jrt"))); Use Set.of. Paul.
On 13 Apr 2016, at 20:55, Xueming Shen <xueming.shen@oracle.com> wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
issue: https://bugs.openjdk.java.net/browse/JDK-8147460 webrev: http://cr.openjdk.java.net/~sherman/8147460/webrev
Simple benchmark [1] has been run both on jimage and the "exploded modules" directory, the numbers suggest we also get an encouraging performance boost on most of the frequently invoked access methods.
[1] http://cr.openjdk.java.net/~sherman/8147460/MyBenchmark.java
Benchmark Mode Cnt Score Error Units ------------------------------------------------------------ [jimage/existing] MyBenchmark.testExists avgt 50 18.592 ± 1.213 ms/op MyBenchmark.testIsDirectory avgt 50 17.382 ± 1.178 ms/op MyBenchmark.testIsRegularFile avgt 50 18.576 ± 1.577 ms/op MyBenchmark.testItr avgt 50 58.414 ± 1.638 ms/op MyBenchmark.testLookupPath avgt 50 18.935 ± 1.093 ms/op MyBenchmark.testLookupStr avgt 50 38.597 ± 1.663 ms/op MyBenchmark.testToRealPath avgt 50 30.119 ± 1.196 ms/op
[jimage/new] MyBenchmark.testExists avgt 50 10.865 ± 1.125 ms/op MyBenchmark.testIsDirectory avgt 50 11.077 ± 1.101 ms/op MyBenchmark.testIsRegularFile avgt 50 10.967 ± 1.220 ms/op MyBenchmark.testItr avgt 50 49.249 ± 3.753 ms/op MyBenchmark.testLookupPath avgt 50 10.857 ± 1.071 ms/op MyBenchmark.testLookupStr avgt 50 34.367 ± 1.341 ms/op MyBenchmark.testToRealPath avgt 50 11.460 ± 1.081 ms/op
------------------------------------------------------------ [exploded dir/existing] MyBenchmark.testExists avgt 50 23.055 ± 1.707 ms/op MyBenchmark.testIsDirectory avgt 50 23.985 ± 1.593 ms/op MyBenchmark.testIsRegularFile avgt 50 24.200 ± 1.744 ms/op MyBenchmark.testItr avgt 50 210.002 ± 6.954 ms/op MyBenchmark.testLookupPath avgt 50 23.242 ± 1.799 ms/op MyBenchmark.testLookupStr avgt 50 43.026 ± 1.751 ms/op MyBenchmark.testToRealPath avgt 50 56.536 ± 1.679 ms/op
[exploded dir/new] MyBenchmark.testExists avgt 50 11.260 ± 1.209 ms/op MyBenchmark.testIsDirectory avgt 50 11.702 ± 1.069 ms/op MyBenchmark.testIsRegularFile avgt 50 12.284 ± 1.333 ms/op MyBenchmark.testItr avgt 50 49.342 ± 1.516 ms/op MyBenchmark.testLookupPath avgt 50 11.041 ± 1.051 ms/op MyBenchmark.testLookupStr avgt 50 35.850 ± 1.618 ms/op MyBenchmark.testToRealPath avgt 50 13.016 ± 1.339 ms/op
Thanks, Sherman
jrt-fs.jar code has to run on jdk8 as well. Cannot use jdk9 API. -Sundar On 4/14/2016 2:23 PM, Paul Sandoz wrote:
Hi Sherman,
Not a full review, I browsed the code very quickly and noticed one thing:
JrtFileSystem —
167 private static final Set<String> supportedFileAttributeViews 168 = Collections.unmodifiableSet( 169 new HashSet<String>(Arrays.asList("basic", "jrt")));
Use Set.of.
Paul.
On 13 Apr 2016, at 20:55, Xueming Shen <xueming.shen@oracle.com> wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
issue: https://bugs.openjdk.java.net/browse/JDK-8147460 webrev: http://cr.openjdk.java.net/~sherman/8147460/webrev
Simple benchmark [1] has been run both on jimage and the "exploded modules" directory, the numbers suggest we also get an encouraging performance boost on most of the frequently invoked access methods.
[1] http://cr.openjdk.java.net/~sherman/8147460/MyBenchmark.java
Benchmark Mode Cnt Score Error Units ------------------------------------------------------------ [jimage/existing] MyBenchmark.testExists avgt 50 18.592 ± 1.213 ms/op MyBenchmark.testIsDirectory avgt 50 17.382 ± 1.178 ms/op MyBenchmark.testIsRegularFile avgt 50 18.576 ± 1.577 ms/op MyBenchmark.testItr avgt 50 58.414 ± 1.638 ms/op MyBenchmark.testLookupPath avgt 50 18.935 ± 1.093 ms/op MyBenchmark.testLookupStr avgt 50 38.597 ± 1.663 ms/op MyBenchmark.testToRealPath avgt 50 30.119 ± 1.196 ms/op
[jimage/new] MyBenchmark.testExists avgt 50 10.865 ± 1.125 ms/op MyBenchmark.testIsDirectory avgt 50 11.077 ± 1.101 ms/op MyBenchmark.testIsRegularFile avgt 50 10.967 ± 1.220 ms/op MyBenchmark.testItr avgt 50 49.249 ± 3.753 ms/op MyBenchmark.testLookupPath avgt 50 10.857 ± 1.071 ms/op MyBenchmark.testLookupStr avgt 50 34.367 ± 1.341 ms/op MyBenchmark.testToRealPath avgt 50 11.460 ± 1.081 ms/op
------------------------------------------------------------ [exploded dir/existing] MyBenchmark.testExists avgt 50 23.055 ± 1.707 ms/op MyBenchmark.testIsDirectory avgt 50 23.985 ± 1.593 ms/op MyBenchmark.testIsRegularFile avgt 50 24.200 ± 1.744 ms/op MyBenchmark.testItr avgt 50 210.002 ± 6.954 ms/op MyBenchmark.testLookupPath avgt 50 23.242 ± 1.799 ms/op MyBenchmark.testLookupStr avgt 50 43.026 ± 1.751 ms/op MyBenchmark.testToRealPath avgt 50 56.536 ± 1.679 ms/op
[exploded dir/new] MyBenchmark.testExists avgt 50 11.260 ± 1.209 ms/op MyBenchmark.testIsDirectory avgt 50 11.702 ± 1.069 ms/op MyBenchmark.testIsRegularFile avgt 50 12.284 ± 1.333 ms/op MyBenchmark.testItr avgt 50 49.342 ± 1.516 ms/op MyBenchmark.testLookupPath avgt 50 11.041 ± 1.051 ms/op MyBenchmark.testLookupStr avgt 50 35.850 ± 1.618 ms/op MyBenchmark.testToRealPath avgt 50 13.016 ± 1.339 ms/op
Thanks, Sherman
On 14 Apr 2016, at 11:09, Sundararajan Athijegannathan <sundararajan.athijegannathan@oracle.com> wrote:
jrt-fs.jar code has to run on jdk8 as well. Cannot use jdk9 API.
Ah, ok. Thanks, Paul.
On 14/04/2016 09:53, Paul Sandoz wrote:
Hi Sherman,
Not a full review, I browsed the code very quickly and noticed one thing:
JrtFileSystem —
167 private static final Set<String> supportedFileAttributeViews 168 = Collections.unmodifiableSet( 169 new HashSet<String>(Arrays.asList("basic", "jrt")));
Use Set.of. One constraint that Sherman didn't mention is that jrtfs is compiled to JDK N-1, currently JDK 8, so that tools/IDEs can run on JDK 8 and access the classes/resources in a target JDK 9 runtime image. I thought we put a comment at the top of each source file on this but I need to check. It will be caught by the build anyway.
-Alan.
Looks good -Sundar On 4/14/2016 12:25 AM, Xueming Shen wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
issue: https://bugs.openjdk.java.net/browse/JDK-8147460 webrev: http://cr.openjdk.java.net/~sherman/8147460/webrev
Simple benchmark [1] has been run both on jimage and the "exploded modules" directory, the numbers suggest we also get an encouraging performance boost on most of the frequently invoked access methods.
[1] http://cr.openjdk.java.net/~sherman/8147460/MyBenchmark.java
Benchmark Mode Cnt Score Error Units ------------------------------------------------------------ [jimage/existing] MyBenchmark.testExists avgt 50 18.592 ± 1.213 ms/op MyBenchmark.testIsDirectory avgt 50 17.382 ± 1.178 ms/op MyBenchmark.testIsRegularFile avgt 50 18.576 ± 1.577 ms/op MyBenchmark.testItr avgt 50 58.414 ± 1.638 ms/op MyBenchmark.testLookupPath avgt 50 18.935 ± 1.093 ms/op MyBenchmark.testLookupStr avgt 50 38.597 ± 1.663 ms/op MyBenchmark.testToRealPath avgt 50 30.119 ± 1.196 ms/op
[jimage/new] MyBenchmark.testExists avgt 50 10.865 ± 1.125 ms/op MyBenchmark.testIsDirectory avgt 50 11.077 ± 1.101 ms/op MyBenchmark.testIsRegularFile avgt 50 10.967 ± 1.220 ms/op MyBenchmark.testItr avgt 50 49.249 ± 3.753 ms/op MyBenchmark.testLookupPath avgt 50 10.857 ± 1.071 ms/op MyBenchmark.testLookupStr avgt 50 34.367 ± 1.341 ms/op MyBenchmark.testToRealPath avgt 50 11.460 ± 1.081 ms/op
------------------------------------------------------------ [exploded dir/existing] MyBenchmark.testExists avgt 50 23.055 ± 1.707 ms/op MyBenchmark.testIsDirectory avgt 50 23.985 ± 1.593 ms/op MyBenchmark.testIsRegularFile avgt 50 24.200 ± 1.744 ms/op MyBenchmark.testItr avgt 50 210.002 ± 6.954 ms/op MyBenchmark.testLookupPath avgt 50 23.242 ± 1.799 ms/op MyBenchmark.testLookupStr avgt 50 43.026 ± 1.751 ms/op MyBenchmark.testToRealPath avgt 50 56.536 ± 1.679 ms/op
[exploded dir/new] MyBenchmark.testExists avgt 50 11.260 ± 1.209 ms/op MyBenchmark.testIsDirectory avgt 50 11.702 ± 1.069 ms/op MyBenchmark.testIsRegularFile avgt 50 12.284 ± 1.333 ms/op MyBenchmark.testItr avgt 50 49.342 ± 1.516 ms/op MyBenchmark.testLookupPath avgt 50 11.041 ± 1.051 ms/op MyBenchmark.testLookupStr avgt 50 35.850 ± 1.618 ms/op MyBenchmark.testToRealPath avgt 50 13.016 ± 1.339 ms/op
Thanks, Sherman
On 13/04/2016 19:55, Xueming Shen wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
I'm skimmed through the changes and it looks very good. My only concern is that we might have not enough tests for jrtfs to exercise it completely. I just checked the coverage it is indeed quite low. I'll create a bug for this. In passing, in JrtDirectoryStream then the spec allows for read ahead and so the original implementation was okay to return this when closed. What you have is fine, just pointing out that both behaviors are allowed. -Alan
On 04/14/2016 11:57 AM, Alan Bateman wrote:
On 13/04/2016 19:55, Xueming Shen wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
I'm skimmed through the changes and it looks very good.
My only concern is that we might have not enough tests for jrtfs to exercise it completely. I just checked the coverage it is indeed quite low. I'll create a bug for this.
In passing, in JrtDirectoryStream then the spec allows for read ahead and so the original implementation was okay to return this when closed. What you have is fine, just pointing out that both behaviors are allowed.
Thanks Alan, There is a test that passing a null filter into Files.newDirectoryStream(path, filter) and then FileSystemProvider.newDirectoryStream(...), to mean "no filter". My reading of the spec suggests it should get a NPE. (to use the nweDirectoryStream(dir) for accept all). Can you confirm? Sherman
On 14/04/2016 20:07, Xueming Shen wrote:
:
There is a test that passing a null filter into Files.newDirectoryStream(path, filter) and then FileSystemProvider.newDirectoryStream(...), to mean "no filter". My reading of the spec suggests it should get a NPE. (to use the nweDirectoryStream(dir) for accept all). Can you confirm? Yes, it should fail, it does not mean "no filter".
-Alan
On 04/14/2016 11:57 AM, Alan Bateman wrote:
On 13/04/2016 19:55, Xueming Shen wrote:
Hi,
Please hep review the cleanup changes for jrtfs implementation.
I'm skimmed through the changes and it looks very good.
My only concern is that we might have not enough tests for jrtfs to exercise it completely. I just checked the coverage it is indeed quite low. I'll create a bug for this.
I did not realize that the PathOps.java does not include all the path tests we have in its nio version. I updated it with all of the "unix version" tests. It did trigger couple corner cases (the diff against yesterday's version in JrtPath.java attached ) http://cr.openjdk.java.net/~sherman/8147460/webrev/ sherman ---------------------------------------------------------------------------------------------------------------------- JrtPath.java: 91,93c91 < if (path.length() == 0) < return this; < if (path.length() == 1 && path.charAt(0) == '/') ---
if (path.length() == 0 || path.length() == 1 && path.charAt(0) == '/')
210,212d207 < if (path.length() == 0) { < return o; < } 240a236
int pos = 0;
243c239 < if (sb.length() < len) { // no tailing slash at the end ---
if (pos < len) { // no tailing slash at the end
267c263 < if (this.path.length() == 0 || o.isAbsolute()) { ---
if (o.isAbsolute()) {
270,272d265 < if (o.path.length() == 0) { < return this; < } 299,301d291 < if (off == 0) { < return tp.length() == 0; < }
On 15/04/2016 00:44, Xueming Shen wrote:
I did not realize that the PathOps.java does not include all the path tests we have in its nio version. I updated it with all of the "unix version" tests. It did trigger couple corner cases (the diff against yesterday's version in JrtPath.java attached )
Thanks, it good to have that test updated as there are many corner cases. A few other areas that don't seem to have tests is the jrt implementation of FileStore and regex usage with DirectoryStream but we can expand on this once you get this update in. -Alan
participants (4)
-
Alan Bateman
-
Paul Sandoz
-
Sundararajan Athijegannathan
-
Xueming Shen