FilePermission Canonical path optimization

[Peter Levart]

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