[9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/ This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now. Thanks Max
Hi Max, The code looks ok. How much faster does it make FilePermission compares? I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10. Roger On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
Webrev updated at http://cr.openjdk.java.net/~weijun/8177969/webrev.01/ Changes since webrev.00 [1] 1. Copyright years. 2. Test fix. check() should include Windows drive tests like contains() does. Thanks Max [1] http://cr.openjdk.java.net/~weijun/8177969/webrev.01/interdiff.patch.html On 04/05/2017 10:13 PM, Roger Riggs wrote:
Hi Max,
The code looks ok.
How much faster does it make FilePermission compares?
I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10.
Roger
On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
This fix looks good to me. However, I would suggest adding some additional comments to the body of the containsPath method explaining what it is doing so that it is easier to understand. --Sean On 4/7/17 10:50 AM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.01/
Changes since webrev.00 [1]
1. Copyright years.
2. Test fix. check() should include Windows drive tests like contains() does.
Thanks Max
[1] http://cr.openjdk.java.net/~weijun/8177969/webrev.01/interdiff.patch.html
On 04/05/2017 10:13 PM, Roger Riggs wrote:
Hi Max,
The code looks ok.
How much faster does it make FilePermission compares?
I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10.
Roger
On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
Webrev updated at http://cr.openjdk.java.net/~weijun/8177969/webrev.02/ Changes since webrev.01: 1. Comments. 2. Another enhancement when I am writing comments. Since we can be sure that a path has no ".." by only looking at its head, we can also be sure that a path is all ".." by only looking at its tail. JPRT core+jaxp runs fine. JCK tests run fine. Now benchmark result (after adjusting the granted permission to "../../../../-") is: Benchmark Mode Cnt Score Error Units checkReadNewUnfixed thrpt 15 174963.698 ± 5952.239 ops/s checkReadNewFixed00 thrpt 15 417692.698 ± 24994.735 ops/s checkReadNewFixed02 thrpt 15 447824.218 ± 20199.194 ops/s checkReadOld200 thrpt 15 425353.705 ± 3546.411 ops/s checkReadOld201 thrpt 15 2045.070 ± 57.287 ops/s This time checkReadNewFixed02 looks even better than checkReadOld200 running on jdk8u131. I dare not run it again. :-) Thanks Max On 04/08/2017 04:48 AM, Sean Mullan wrote:
This fix looks good to me. However, I would suggest adding some additional comments to the body of the containsPath method explaining what it is doing so that it is easier to understand.
--Sean
On 4/7/17 10:50 AM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.01/
Changes since webrev.00 [1]
1. Copyright years.
2. Test fix. check() should include Windows drive tests like contains() does.
Thanks Max
[1] http://cr.openjdk.java.net/~weijun/8177969/webrev.01/interdiff.patch.html
On 04/05/2017 10:13 PM, Roger Riggs wrote:
Hi Max,
The code looks ok.
How much faster does it make FilePermission compares?
I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10.
Roger
On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
The updated webrev looks good. Thanks, Sean On 4/7/17 8:39 PM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.02/
Changes since webrev.01:
1. Comments.
2. Another enhancement when I am writing comments. Since we can be sure that a path has no ".." by only looking at its head, we can also be sure that a path is all ".." by only looking at its tail.
JPRT core+jaxp runs fine. JCK tests run fine.
Now benchmark result (after adjusting the granted permission to "../../../../-") is:
Benchmark Mode Cnt Score Error Units checkReadNewUnfixed thrpt 15 174963.698 ± 5952.239 ops/s checkReadNewFixed00 thrpt 15 417692.698 ± 24994.735 ops/s checkReadNewFixed02 thrpt 15 447824.218 ± 20199.194 ops/s checkReadOld200 thrpt 15 425353.705 ± 3546.411 ops/s checkReadOld201 thrpt 15 2045.070 ± 57.287 ops/s
This time checkReadNewFixed02 looks even better than checkReadOld200 running on jdk8u131. I dare not run it again. :-)
Thanks Max
On 04/08/2017 04:48 AM, Sean Mullan wrote:
This fix looks good to me. However, I would suggest adding some additional comments to the body of the containsPath method explaining what it is doing so that it is easier to understand.
--Sean
On 4/7/17 10:50 AM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.01/
Changes since webrev.00 [1]
1. Copyright years.
2. Test fix. check() should include Windows drive tests like contains() does.
Thanks Max
[1] http://cr.openjdk.java.net/~weijun/8177969/webrev.01/interdiff.patch.html
On 04/05/2017 10:13 PM, Roger Riggs wrote:
Hi Max,
The code looks ok.
How much faster does it make FilePermission compares?
I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10.
Roger
On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
+1, looks good. Thanks, Roger On 4/10/2017 10:08 AM, Sean Mullan wrote:
The updated webrev looks good.
Thanks, Sean
On 4/7/17 8:39 PM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.02/
Changes since webrev.01:
1. Comments.
2. Another enhancement when I am writing comments. Since we can be sure that a path has no ".." by only looking at its head, we can also be sure that a path is all ".." by only looking at its tail.
JPRT core+jaxp runs fine. JCK tests run fine.
Now benchmark result (after adjusting the granted permission to "../../../../-") is:
Benchmark Mode Cnt Score Error Units checkReadNewUnfixed thrpt 15 174963.698 ± 5952.239 ops/s checkReadNewFixed00 thrpt 15 417692.698 ± 24994.735 ops/s checkReadNewFixed02 thrpt 15 447824.218 ± 20199.194 ops/s checkReadOld200 thrpt 15 425353.705 ± 3546.411 ops/s checkReadOld201 thrpt 15 2045.070 ± 57.287 ops/s
This time checkReadNewFixed02 looks even better than checkReadOld200 running on jdk8u131. I dare not run it again. :-)
Thanks Max
On 04/08/2017 04:48 AM, Sean Mullan wrote:
This fix looks good to me. However, I would suggest adding some additional comments to the body of the containsPath method explaining what it is doing so that it is easier to understand.
--Sean
On 4/7/17 10:50 AM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8177969/webrev.01/
Changes since webrev.00 [1]
1. Copyright years.
2. Test fix. check() should include Windows drive tests like contains() does.
Thanks Max
[1] http://cr.openjdk.java.net/~weijun/8177969/webrev.01/interdiff.patch.html
On 04/05/2017 10:13 PM, Roger Riggs wrote:
Hi Max,
The code looks ok.
How much faster does it make FilePermission compares?
I assume if it is not accepted to be fixed in JDK 9, you will push it to JDK 10.
Roger
On 4/3/2017 11:30 AM, Weijun Wang wrote:
http://cr.openjdk.java.net/~weijun/8177969/webrev.00/
This new implementation of containsPath(Path,Path) uses the logic of Path::relativize without directly calling it, which is much faster now.
Thanks Max
participants (3)
-
Roger Riggs
-
Sean Mullan
-
Weijun Wang