Looks fine to me. Thanks, Xuelei
On 1/13/2016 3:24 AM, Sean Mullan wrote: > New (simpler) fix using Map.put(): > > http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.02/ > > This should be ok, as the JDK 8 code and the JDK 9 code prior to the fix > for 8055753 [1] was using Map.put instead of putIfAbsent. > > Thanks, > Sean > > [1] https://bugs.openjdk.java.net/browse/JDK-8055753 > > On 01/12/2016 11:02 AM, Sean Mullan wrote: >> On 01/12/2016 10:26 AM, Xuelei Fan wrote: >>> I think hashMap.put() may be similar or more effective than >>> hashMap.putIfAbsent(). >> >> Ok, I think I see what you are trying to say. >> >> The common case is that the putIfAbsent method is going to succeed (i.e. >> the entry will be absent). It may not succeed in cases where there are >> multiple threads simultaneously computing the initial permissions for >> the same PD or the value is garbage collected due to memory demand. >> >> So, I think the question boils down to whether put is faster than >> putIfAbsent in the common case (when there is not an entry). Probably >> yes, since it doesn't have to check if there is an entry already. >> >>> Is there a case that the key/weakPd is the same, >>> but the value/pc is different? >> >> I believe the pc should always be the same. If you change the policy >> file(s), then you have to explicitly refresh it (using Policy.refresh()) >> which should cause a new cache to be created. >> >> Let me do some more testing around this, and see if I can check >> performance and see what is better. >> >> Thanks, >> Sean >> >>> >>> Xuelei >>> >>> On 1/12/2016 11:01 PM, Sean Mullan wrote: >>>> Hi Ivan, >>>> >>>> On 01/12/2016 09:38 AM, Ivan Gerasimov wrote: >>>>> Hi Sean! >>>>> >>>>> On 12.01.2016 16:26, Sean Mullan wrote: >>>>>> I received a private comment that there may be cases where the map's >>>>>> value is reclaimed by the garbage collector, but the key still >>>>>> exists. >>>>>> If that ProtectionDomain is subsequently used for permission >>>>>> checks, a >>>>>> Map.putIfAbsent method will not replace the value. >>>>>> >>>>>> So, I have added an additional check for this case in the PDCache.put >>>>>> method, and it instead uses the Map.replace method. Updated webrev: >>>>>> >>>>> >>>>> Wouldn't it be a little bit more efficient to use merge() here? >>>>> >>>>> Something like: >>>>> pdMap.merge(weakPd, v, (pv, nv) -> pv.get() != null ? pv : nv); >>>> >>>> Yes - good catch! >>>> >>>> Will update and send out new webrev after a round of testing ... >>>> >>>> Thanks, >>>> Sean >>>> >>>>> >>>>> Sincerely yours, >>>>> Ivan >>>>> >>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.01/ >>>>>> >>>>>> --Sean >>>>>> >>>>>> On 01/08/2016 03:06 PM, Sean Mullan wrote: >>>>>>> Please review this fix for a memory leak in the ProtectionDomain >>>>>>> cache >>>>>>> (which maps each ProtectionDomain to their granted >>>>>>> PermissionCollection). The memory leak only occurs if custom >>>>>>> permissions >>>>>>> are used in a policy file. >>>>>>> >>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.00/ >>>>>>> >>>>>>> Custom permissions cause a chain of strong references that link >>>>>>> back to >>>>>>> the ProtectionDomain key used in the map, preventing it from being >>>>>>> removed (and also preventing the custom permission's URLClassLoader >>>>>>> from >>>>>>> being garbage collected). This fix wraps the PermissionCollection >>>>>>> in a >>>>>>> SoftReference which allows the objects to be garbage collected in >>>>>>> response to memory demand. >>>>>>> >>>>>>> Thanks, >>>>>>> Sean >>>>>> >>>>> >>>
