Here is the benchmark results:
Benchmark Mode Cnt Score Error Units
SubjectBenchmark.jdkSubject thrpt 20 473086.025 ± 7661.215 ops/s
SubjectBenchmark.patchedSubject thrpt 20 2529016.530 ± 50982.465 ops/s
On a one hand, patched version has 5x higher throughput. However, on an other
hand
the throughput of the original code is sufficient for any real application. The
benchmark code is attached.
Sorry for the noise. Should have done benchmarks before trying to optimize it.
Tigran.
----- Original Message -----
> From: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
> To: "Weijun Wang" <weijun.w...@oracle.com>
> Cc: "security-dev" <security-dev@openjdk.java.net>
> Sent: Friday, April 24, 2020 12:50:03 PM
> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
> I read it differently:
>
> Attempting to query the presence of an ineligible element may throw an
> exception, or it may simply return false; some implementations
> will exhibit the former behavior and some will exhibit the latter.
>
> Implementation might thrown an exception or return false, but sill don't allow
> null values.
>
> ... would not result in the insertion of an ineligible element into the
> collection may throw an exception or it may
> succeed, at the option of the implementation.
>
>
> For example, TreeSet throws NPE depending on Comparator.
>
> Well, I understand, this is a corner case that possibly not worth to
> optimize.
> Let me write
> a benchmark to see how much difference it makes.
>
> Tigran.
>
>
> ----- Original Message -----
>> From: "Weijun Wang" <weijun.w...@oracle.com>
>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>> Cc: "security-dev" <security-dev@openjdk.java.net>
>> Sent: Friday, April 24, 2020 12:23:48 PM
>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>
>> My understanding is by optional you have 2 options while designing the Set:
>>
>> 1. Allow null
>>
>> 2. Not allow null
>>
>> Now that SecureSet has chosen the 2nd way, it is now in the "this set does
>> not
>> permit null elements" category and therefore it should "@throws
>> NullPointerException if the specified element is null".
>>
>> --Max
>>
>>> On Apr 24, 2020, at 6:13 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>>> wrote:
>>>
>>> Hi Max,
>>>
>>> that's right, but java doc as well mentions that this exception is
>>> *optional*,
>>> and has a corresponding note:
>>>
>>> Some collection implementations have restrictions on the elements that they
>>> may
>>> contain. For example, some implementations prohibit null elements, and some
>>> have restrictions on the types of their elements. Attempting to add an
>>> ineligible element throws an unchecked exception, typically
>>> NullPointerException or ClassCastException. Attempting to query the
>>> presence of
>>> an ineligible element may throw an exception, or it may simply return false;
>>> some implementations will exhibit the former behavior and some will exhibit
>>> the
>>> latter. More generally, attempting an operation on an ineligible element
>>> whose
>>> completion would not result in the insertion of an ineligible element into
>>> the
>>> collection may throw an exception or it may succeed, at the option of the
>>> implementation. Such exceptions are marked as "optional" in the
>>> specification
>>> for this interface.
>>>
>>> Thus, returning *false* is justified and spec compliant.
>>>
>>>
>>> Thanks,
>>> Tigran.
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>> Sent: Friday, April 24, 2020 11:42:41 AM
>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>
>>>> Hi Tigran,
>>>>
>>>> In java.util.Set, we have:
>>>>
>>>> * @throws NullPointerException if the specified element is null and this
>>>> * set does not permit null elements
>>>> * (<a href="Collection.html#optional-restrictions">optional</a>)
>>>> */
>>>> boolean contains(Object o);
>>>>
>>>> As an implementation, SecureSet must follow the spec to throw an NPE. If it
>>>> returns null, some unexpected thing might happen when the contains()
>>>> method is
>>>> called somewhere else.
>>>>
>>>> Thanks,
>>>> Max
>>>>
>>>>> On Apr 24, 2020, at 4:21 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Dear Java-SE security developers,
>>>>>
>>>>>
>>>>> Imagine a following code:
>>>>>
>>>>> ```
>>>>> Subject s1 = ... ;
>>>>>
>>>>> Subject s2 = ... ;
>>>>>
>>>>>
>>>>> s2.getPrincipals().addAll(s1.getPrincipals());
>>>>>
>>>>> ```
>>>>>
>>>>> The Subject's SecureSet.addAll checks that provided Set doesn't contains
>>>>> 'null'
>>>>> values
>>>>> by calling collectionNullClean, which calls SecureSet#contains:
>>>>>
>>>>> ```
>>>>> try {
>>>>> hasNullElements = coll.contains(null);
>>>>> } catch (NullPointerException npe) {
>>>>>
>>>>> ```
>>>>>
>>>>> The SecureSet#contains itself checks for 'null' values, the NPE is always
>>>>> generated.
>>>>>
>>>>> This as introduced by commit e680ab7f208e
>>>>>
>>>>> https://hg.openjdk.java.net/jdk/jdk/diff/e680ab7f208e/jdk/src/share/classes/javax/security/auth/Subject.java
>>>>>
>>>>>
>>>>> As SecureSet doesn't allow null values, it will be much simpler to return
>>>>> false
>>>>> right away:
>>>>>
>>>>> ```
>>>>>
>>>>> public boolean contains(Object o) {
>>>>> if (o == null) {
>>>>> // null values rejected by add
>>>>> return false;
>>>>> }
>>>>>
>>>>> ...
>>>>> }
>>>>>
>>>>> ```
>>>>>
>>>>>
>>>>> Thanks in advance,
> > >>> Tigran.
package dev.kofemann.playground;
import com.sun.security.auth.UnixNumericUserPrincipal;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import javax.security.auth.Subject;
import java.util.Collections;
import java.util.stream.Collectors;
import java.util.stream.LongStream;
@State(Scope.Benchmark)
@BenchmarkMode(Mode.Throughput)
public class SubjectBenchmark {
private Subject jdkSubject;
private Subject2 patchedSubject;
@Setup
public void setUp() {
jdkSubject = new Subject(false,
LongStream.range(0, 10).mapToObj(UnixNumericUserPrincipal::new).collect(Collectors.toSet()),
Collections.emptySet(), Collections.emptySet());
patchedSubject = new Subject2(false,
LongStream.range(0, 10).mapToObj(UnixNumericUserPrincipal::new).collect(Collectors.toSet()),
Collections.emptySet(), Collections.emptySet());
}
@Benchmark
@Fork(value = 2)
@Measurement(iterations = 10, time = 5)
@Warmup(iterations = 5, time = 1)
public void jdkSubject(Blackhole blackhole) {
Subject s = new Subject();
s.getPrincipals().addAll(jdkSubject.getPrincipals());
blackhole.consume(s);
}
@Benchmark
@Fork(value = 2)
@Measurement(iterations = 10, time = 5)
@Warmup(iterations = 5, time = 1)
public void patchedSubject(Blackhole blackhole) {
Subject2 s = new Subject2();
s.getPrincipals().addAll(patchedSubject.getPrincipals());
blackhole.consume(s);
}
}
