RFR 8243592: Subject$SecureSet::contains(null) is suboptimal

Mkrtchyan, Tigran tigran.mkrtchyan at desy.de
Wed Jul 1 08:37:07 UTC 2020



Hi Max,

Thanks for following up on that. I like your approach - it doesn't
introduce confusion or behaviour changes.

I re-run the benchmark:

Benchmark                             Mode  Cnt        Score       Error  Units
SubjectBenchmark.jdkOriginalSubject  thrpt   20   360202.816 ±  4071.762  ops/s
SubjectBenchmark.maxPatchSubject     thrpt   20  1609248.338 ± 48137.227  ops/s
SubjectBenchmark.tigranPatchSubject  thrpt   20  1973207.564 ± 89670.297  ops/s

The performance difference can be ignored.

Best regards,
   Tigran.

P.S. Thanks for the pointer to the github repo!

----- Original Message -----
> From: "Weijun Wang" <weijun.wang at oracle.com>
> To: "Sean Mullan" <sean.mullan at oracle.com>
> Cc: "security-dev" <security-dev at openjdk.java.net>
> Sent: Wednesday, July 1, 2020 5:31:54 AM
> Subject: Re: RFR 8243592: Subject$SecureSet::contains(null) is suboptimal

> Please review an updated fix at
> 
>   https://cr.openjdk.java.net/~weijun/8243592/webrev.03
> 
> After some discussion, we think there is no need to call contains(null) at all
> since it might not be reliable (see the new test). Now, collectionNullClean
> inspects all items in the input collection and create a new LinkedList.
> 
> Since there is no need to repopulate a list inside "new SecureSet", the new
> implementation is not slower than the original proposal from Tigran.
> 
> And there is no need for a CSR because there will be no behavior change.
> 
> Thanks,
> Max
> 
> p.s. Welcome to the github playground at
> https://github.com/openjdk/playground/pull/9, but formal code review should
> stay in this mail thread.
> 
>> On Apr 30, 2020, at 9:50 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>> 
>> Webrev updated at
>> 
>>   http://cr.openjdk.java.net/~weijun/8243592/webrev.02/
>> 
>> I removed my old test and rewrite the existing SubjectNullTests.java test in
>> TestNG with quite some new test cases.
>> 
>> There are so many behavior changes (although we might say good programs are not
>> affected) that I decided to add a CSR and a release note. Please also take a
>> look.
>> 
>>            CSR : https://bugs.openjdk.java.net/browse/JDK-8244165
>>   release note : https://bugs.openjdk.java.net/browse/JDK-8244169
>> 
>> Thanks,
>> Max
>> 
>> 
>>> On Apr 29, 2020, at 2:57 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>> 
>>> I don't think containsAll and retainsAll need to call collectionNullClean either
>>> because SecureSet.contains(null) returns false now.
>>> 
>>> --Sean
>>> 
>>> On 4/28/20 9:58 AM, Weijun Wang wrote:
>>>> A new webrev is at
>>>>   http://cr.openjdk.java.net/~weijun/8243592/webrev.01/
>>>> I take this chance to do some formatting and add a new test. My first testng
>>>> test!
>>>> Thanks,
>>>> Max
>>>>> On Apr 28, 2020, at 8:16 PM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>>>> 
>>>>> On 4/27/20 10:39 PM, Weijun Wang wrote:
>>>>>> Reading the Set spec, it looks like an NPE is still needed for add(), but
>>>>>> remove() can be modified.
>>>>> 
>>>>> Good point, I agree.
>>>>> 
>>>>> --Sean
>>>>> 
>>>>>> --Max
>>>>>>> On Apr 27, 2020, at 10:27 PM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>>>>>> 
>>>>>>> The fix looks fine to me. For consistency, you could make the same change for
>>>>>>> null elements in the other SecureSet methods: add, remove.
>>>>>>> 
>>>>>>> --Sean
>>>>>>> 
>>>>>>> On 4/25/20 3:39 AM, Weijun Wang wrote:
>>>>>>>> Please take a review at
>>>>>>>>   http://cr.openjdk.java.net/~weijun/8243592/webrev.00/
>>>>>>>> This is helpful if we do any set arithmetic between 2 Subject objects.
>>>>>>>> No new regression test, I intend to add a noreg-trivial label.
>>>>>>>> *Tigran*: Please confirm you are OK with the "Contributed-by" line.
>>>>>>>> Thanks,
>>>>>>>> Max



More information about the security-dev mailing list