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