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.w...@oracle.com>
> To: "Sean Mullan" <sean.mul...@oracle.com>
> Cc: "security-dev" <security-dev@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.w...@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.mul...@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.mul...@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.mul...@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
