On Tue, 22 Jun 2021 02:46:02 GMT, Dongbo He <dongb...@openjdk.org> wrote:
>> Now AlgorithmConstraints:checkAlgorithm uses List to check if an algorithm
>> has been disabled. It is less efficient when there are more disabled
>> elements in the list, we can use Set instead of List to speed up the search.
>>
>> Patch contains a benchmark that can be run with `make test
>> TEST="micro:java.security.AlgorithmConstraintsPermits"`.
>> Baseline results before patch:
>>
>> Benchmark (algorithm) Mode Cnt Score
>> Error Units
>> AlgorithmConstraintsPermits.permits SSLv3 avgt 5 21.687 ?
>> 1.118 ns/op
>> AlgorithmConstraintsPermits.permits DES avgt 5 324.216 ?
>> 6.233 ns/op
>> AlgorithmConstraintsPermits.permits NULL avgt 5 709.462 ?
>> 51.259 ns/op
>> AlgorithmConstraintsPermits.permits TLS1.3 avgt 5 687.497 ?
>> 170.181 ns/op
>>
>> Benchmark results after patch:
>>
>> Benchmark (algorithm) Mode Cnt Score
>> Error Units
>> AlgorithmConstraintsPermits.permits SSLv3 avgt 5 46.407 ?
>> 1.057 ns/op
>> AlgorithmConstraintsPermits.permits DES avgt 5 65.722 ?
>> 0.578 ns/op
>> AlgorithmConstraintsPermits.permits NULL avgt 5 43.988 ?
>> 1.264 ns/op
>> AlgorithmConstraintsPermits.permits TLS1.3 avgt 5 399.546 ?
>> 11.194 ns/op
>>
>> SSLv3, DES, NULL are the first, middle and last element in
>> `jdk.tls.disabledAlgorithms` from `conf/security/java.security`.
>>
>> Tomcat(maxKeepAliveRequests=1, which will disable HTTP/1.0
>> keep-alive)+Jmeter:
>> Before patch:
>>
>> summary + 50349 in 00:00:30 = 1678.4/s Avg: 238 Min: 188 Max: 298
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 135183 in 00:01:22 = 1654.5/s Avg: 226 Min: 16 Max: 1281
>> Err: 0 (0.00%)
>> summary + 50240 in 00:00:30 = 1674.1/s Avg: 238 Min: 200 Max: 308
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 185423 in 00:01:52 = 1659.7/s Avg: 229 Min: 16 Max: 1281
>> Err: 0 (0.00%)
>> summary + 50351 in 00:00:30 = 1678.4/s Avg: 238 Min: 191 Max: 306
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 235774 in 00:02:22 = 1663.7/s Avg: 231 Min: 16 Max: 1281
>> Err: 0 (0.00%)
>> summary + 50461 in 00:00:30 = 1681.9/s Avg: 237 Min: 174 Max: 303
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>>
>> After patch:
>>
>> summary + 59003 in 00:00:30 = 1966.6/s Avg: 203 Min: 158 Max: 272
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 146675 in 00:01:18 = 1884.6/s Avg: 198 Min: 26 Max: 697
>> Err: 0 (0.00%)
>> summary + 58965 in 00:00:30 = 1965.9/s Avg: 203 Min: 166 Max: 257
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 205640 in 00:01:48 = 1907.2/s Avg: 199 Min: 26 Max: 697
>> Err: 0 (0.00%)
>> summary + 59104 in 00:00:30 = 1969.1/s Avg: 203 Min: 157 Max: 266
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 264744 in 00:02:18 = 1920.7/s Avg: 200 Min: 26 Max: 697
>> Err: 0 (0.00%)
>> summary + 59323 in 00:00:30 = 1977.6/s Avg: 202 Min: 158 Max: 256
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>>
>>
>> Testing: tier1, tier2
>
> Dongbo He has updated the pull request incrementally with one additional
> commit since the last revision:
>
> refactor the code && fix some errors
src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
line 133:
> 131:
> 132: // Check for alias
> 133: Pattern INCLUDE_PATTERN = Pattern.compile(
You may miss my previous comment. This variable is not static, hence all
capital letters naming does not apply here.
As you are already here, it may be nice if you'd like to have this variable as
a static final class field. Then, the compile() will be called one time at
most for this class. As a static class field, you could use the capitalized
name.
src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
line 139:
> 137: if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) {
> 138: disabledAlgorithms.remove(matcher.group());
> 139:
> disabledAlgorithms.addAll(getAlgorithms(PROPERTY_DISABLED_EC_CURVES));
I guess this line exceed 80 characters? Please keep it in 80 characters for
each line.
src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
line 139:
> 137: if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) {
> 138: disabledAlgorithms.remove(matcher.group());
> 139:
> disabledAlgorithms.addAll(getAlgorithms(PROPERTY_DISABLED_EC_CURVES));
It may increase the readability a little bit if having the assignment in the
declaration line:
- Matcher matcher;
- if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) {
+ Matcher matcher = INCLUDE_PATTERN.matcher(s);
+ if (matcher.matches()) {
-------------
PR: https://git.openjdk.java.net/jdk/pull/4424
