RFR: 8072692: Improve performance of SecurityManager.checkPackageAccess

[Sean Mullan]

On 06/17/2015 03:27 AM, Daniel Fuchs wrote:
> Hi Max,
>
> On 6/17/15 2:42 AM, Weijun Wang wrote:
>> 1478 final int plast = restrictedPkg.length() - 1;
>>
>> Why is it named plast?
>
> Right. That's a bad name. Maybe 'rlast' would be more appropriate.
> Any better name for 'the index of the last character in restrictedPkg'?

rlast seems ok to me. I will change it.

>> 1494    //    - we check that restrictedPkg.length is pkg.length + 1,
>> 1495    //    - we check that restrictedPkg starts with pkg,
>> 1496    //    - and we check that the last character in restrictedPkg
>> 1497    //      is '.'
>>
>> Seems redundant. The check below is not difficult to read.

Ok, I will remove those lines.

>> Also, is checking the "." at the end of restrictedPkg useful? On the
>> one hand we know every item in package.access should always end with
>> it. On the other hand, if someone really adds a "sun" there, the 1st
>> part of the check could go wrong (For example, "sunw" matches). IMHO,
>> either we don't check it at all (hoping property is always set
>> correctly), or we always check for it (cover both "sun.tail" and "sun").
>
> Possibly - but that would be a behavioral change. The current test:
>
> plast == plen && restrictedPkg.startsWith(pkg) &&  restrictedPkg.charAt(plast) == '.'
>
>
> is strictly equivalent to the old test:
>
> restrictedPkg.equals(pkg + ".")

Right, and this is an interesting observation. However, because it is a 
behavior change and there may be potential compatibility issues, I think 
we should open a separate issue to do that.

Any other comments, Max?

--Sean

>
>
> (side note: pkg + "." was the root of the perf issue).
>
>
> best regards,
>
> -- daniel
>
>>
>> Thanks
>> Max
>>
>> On 06/16/2015 10:54 PM, Sean Mullan wrote:
>>> This is the sixth in a series of fixes for JEP 232 (Improve Secure
>>> Application Performance) [1].
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8072692/webrev.00/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8072692
>>>
>>> This fix adds several optimizations to the package matching algorithm
>>> used by the SecurityManager.checkPackageAcccess method. These
>>> improvements result in a 5-7x increase in throughput of this method. A
>>> performance chart has been attached to the bug with more information.
>>>
>>> A new test is included which uses a state machine to verify that the
>>> matching algorithm is working correctly.
>>>
>>> Special thanks to Daniel Fuchs for contributing this fix and the test.
>>>
>>> Thanks,
>>> Sean
>>>
>>> [1] http://openjdk.java.net/jeps/232
>