Quantcast

[9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

Weijun Wang
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

roger riggs
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

Weijun Wang
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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

Sean Mullan
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
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

Weijun Wang
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
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

Sean Mullan
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
>>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR 8177969: Faster FilePermission::implies by avoiding the use of Path::relativize

roger riggs
+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
>>>>>

Loading...