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.mul...@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
