FilePermission Canonical path optimization

[deven you]

Hi All,

I have updated the patch[1] according to above discussion. Please review it.

Thanks a lot

[1] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.03/


2015-02-03 16:04 GMT+08:00 Peter Levart <peter.levart at gmail.com>:

> Hi Deven,
>
>
> On 02/03/2015 08:42 AM, deven you wrote:
>
>> Hi Sean,
>>
>> The performance degradation was reported by creating an object with always
>> getting its canonical path and there is no degradation heard after we made
>> the lazy load  patch for the canonical path.
>>
>> I have asked related people to give me an env to create this problem so
>> that I can take a close look how the application uses FilePermissions and
>> may report my analysis later.
>>
>> However, as far as I know at present, lazy loading the canonical path
>> should be a relative better solution:
>>
>> 1. fast set up time
>> 2. at run time, only necessary, the canonical path will be retrieved, the
>> best case is no canonical path used at all and the worst case is only take
>> almost the same effort as loading it at start up time.
>> 3. According to FilePermissionCollection, this is also true, the implies
>> method won't need to iterator the whole set of permissions, it will return
>> as soon as a proper permission found.
>>
>> Therefore, from general situation, I think this patch makes sense.
>>
>> To Peter,
>>
>> Even with your approach to change 'cpath' 'directory' and 'recursive' to
>> volatile and prepare their orders so that "directory" and "recursive"
>> first, "cpath" last, there is still some problem in the pattern in
>> equals()
>> and impliesIgnoreMask():
>>
>> if (cpath == null) initCpathDirectoryAndRecursive();
>> ... use 'cpath' or ''directory' or 'recursive' ...
>>
>> cpath could be null but initCpathDirectoryAndRecursive may be already
>> running in another thread therefore initCpathDirectoryAndRecursive might
>> be
>> invoked twice.
>>
>
> Good catch! Usually such redundant idempotent initialization is harmless -
> but it *must* be idempotent. Since it involves canonical path computation
> on the filesystem and the filesystem is a mutable "object", this is not
> guaranteed.
>
>
>> To solve this problem, based on your volatile solution, but still make
>> initCpathDirectoryAndRecursive as synchronized method and add below
>> statement at the beginning of this method:
>> if (cpath != null) {
>>      return;
>> }
>>
>> Even if there is another thread running, initCpathDirecotryAndRecursive()
>> will wait the completion of first thread and check cpath value and then
>> return. This solution ensures the initiating logic is run just once.
>>
>
> Right, double-checked locking (with use of volatiles and correct order of
> initialization) would be the best solution here.
>
> Regards, Peter
>
>
>
>> Thanks a lot!
>>
>>
>> Thanks a lot!
>>
>> 2014-12-18 2:35 GMT+08:00 Sean Mullan <sean.mullan at oracle.com>:
>>
>>  Hi,
>>>
>>> Can you elaborate more on the performance degradation that you are seeing
>>> at startup? Are you seeing this when you are running with or without a
>>> SecurityManager? If without a SecurityManager, can you provide some code
>>> paths/examples? As far as I can see, with the proposed fix you are moving
>>> the performance hit to runtime, and it will be triggered the first time a
>>> FilePermission check is made, and at worst case it may need to
>>> canonicalize
>>> all the FilePermissions in the FilePermissionCollection. Also, with the
>>> latest proposed fix you are potentially making performance worse by
>>> introducing synchronized blocks (which as Peter noted, still have some
>>> race
>>> conditions). I can understand why you want to improve application
>>> startup,
>>> but I want to make sure I understand the use case(s) a little better
>>> first.
>>>
>>> Thanks,
>>> Sean
>>>
>>
>