On Thu, 17 Jun 2021 08:16:42 GMT, Dongbo He <[email protected]> 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:
>
> Replace HashSet with TreeSet
Changes requested by xuelei (Reviewer).
src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
line 90:
> 88: }
> 89:
> 90: Set<String> elements = null;
This line could be placed and merged with line 96-98.
`Set<String> elements = decomposer.decompose(algorithm);`
src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
line 101:
> 99: // check the element of the elements
> 100: for (String element : elements) {
> 101: if (algorithms.contains(algorithm)) {
The check should be placed on the decomposed elements.
- if (algorithms.contains(algorithm))
+ if (algorithms.contains(element))
src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
line 133:
> 131:
> 132: // Check for alias
> 133: Pattern INCLUDE_PATTERN = Pattern.compile("include " +
> PROPERTY_DISABLED_EC_CURVES, Pattern.CASE_INSENSITIVE);
Per the Java code conventions, the variable name could be "includePattern",
rather than using capital all letters.
Please keep each line less than or equal to 80 bytes.
src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
line 134:
> 132: // Check for alias
> 133: Pattern INCLUDE_PATTERN = Pattern.compile("include " +
> PROPERTY_DISABLED_EC_CURVES, Pattern.CASE_INSENSITIVE);
> 134: Matcher matcher;
I think the performance impact should be trivial if moving the "matcher"
declaration into the for-loop block.
src/java.base/share/classes/sun/security/util/LegacyAlgorithmConstraints.java
line 31:
> 29: import java.security.CryptoPrimitive;
> 30: import java.security.Key;
> 31: import java.util.HashSet;
I guess this line could be removed.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4424
