RFR: 8247995: Avoid use of a mapping function in Permissions.getPermissionCollection

[Claes Redestad]

On 2020-06-23 00:30, Roger Riggs wrote:
> Hi Claes,
> 
> Looks good.

Thanks!

> 
> Thanks for the followup.

No problem.

/Claes

> 
> Roger
> 
> On 6/22/20 6:29 PM, Claes Redestad wrote:
>> Hi,
>>
>> inlining the new method actually messed up microbenchmark scores a
>> lot (15-80% overhead). I settled for simplifying the duplicate
>> permsMap.get in the outer method in a way that keeps scores neutral:
>>
>> http://cr.openjdk.java.net/~redestad/8247995/open.01/
>>
>> Benchmark                                    Mode  Cnt   Score Error 
>> Units
>> PermissionsImplies.withPermission            avgt   15  27.857 ± 0.137 
>> ns/op
>> PermissionsImplies.withUnresolvedPermission  avgt   15  27.918 ± 0.178 
>> ns/op
>> PermissionsImplies.withoutPermission         avgt   15   4.559 ± 0.115 
>> ns/op
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2020-06-22 23:54, Claes Redestad wrote:
>>> Hi Roger,
>>>
>>> I prototyped it as such, and saw a 0.4ns/op overhead in the
>>> PermissionImplies.withoutPermission. Thinking about it a bit now, this
>>> should be the rare path taken, since it means the permission is not
>>> implied and the performance largely irrelevant. I'll simplify as you 
>>> suggested.
>>>
>>> /Claes
>>>
>>> On 2020-06-22 23:03, Roger Riggs wrote:
>>>> Hi Claes,
>>>>
>>>> Its correct as is but I would have written it without duplicating 
>>>> the 'permsMap.get(p.getClass())' invocation
>>>> and as a single method (unless the inlining of 
>>>> getPermissionCollection(p,create)) is important.
>>>>
>>>> A patch on top of yours:
>>>>
>>>> diff --git 
>>>> a/src/java.base/share/classes/java/security/Permissions.java 
>>>> b/src/java.base/share/classes/java/security/Permissions.java
>>>> --- a/src/java.base/share/classes/java/security/Permissions.java
>>>> +++ b/src/java.base/share/classes/java/security/Permissions.java
>>>> @@ -229,20 +229,16 @@ implements Serializable
>>>>        */
>>>>       private PermissionCollection 
>>>> getPermissionCollection(Permission p,
>>>> boolean createEmpty) {
>>>> +        PermissionCollection pc = permsMap.get(p.getClass());
>>>>           if (!hasUnresolved && !createEmpty) {
>>>>               // Collection not to be created
>>>> -            return permsMap.get(p.getClass());
>>>> +            return pc;
>>>>           }
>>>> -        PermissionCollection pc = permsMap.get(p.getClass());
>>>>           if (pc != null) {
>>>>               // Collection already created
>>>>               return pc;
>>>>           }
>>>> -        return createPermissionCollection(p, createEmpty);
>>>> -    }
>>>>
>>>> -    private PermissionCollection 
>>>> createPermissionCollection(Permission p,
>>>> - boolean createEmpty) {
>>>>           synchronized (permsMap) {
>>>>               Class<?> c = p.getClass();
>>>>               PermissionCollection pc = permsMap.get(c);
>>>>
>>>> Thanks, Roger
>>>>
>>>>
>>>> On 6/22/20 11:04 AM, Claes Redestad wrote:
>>>>> Hi,
>>>>>
>>>>> this patch fixes a corner-case performance issue with
>>>>> Permissions.implies(Permission) by not needing to allocate a mapper
>>>>> function (or lambda) on each invocation of getPermissionCollection
>>>>> when there are unresolved permissions present.
>>>>>
>>>>> Bug:   https://bugs.openjdk.java.net/browse/JDK-8247995
>>>>> Patch: http://cr.openjdk.java.net/~redestad/8247995/open.00/
>>>>>
>>>>> Testing: tier1-2
>>>>>
>>>>> Thanks!
>>>>>
>>>>> /Claes
>>>>
>