RFR: 8270317: Large Allocation in CipherSuite [v2]
Xue-Lei Andrew Fan
xuelei at openjdk.java.net
Thu Jul 22 05:22:49 UTC 2021
On Wed, 21 Jul 2021 23:58:15 GMT, Clive Verghese <cverghese at openjdk.org> wrote:
>> ### Benchmark results
>>
>> I have benchmarked 3 cases.
>>
>> 1. The current situation.
>>
>> Benchmark (cipherSuite) Mode Cnt Score Error Units
>> CipherSuiteBench.benchmarkCipherSuite TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op
>>
>>
>> 2. Use `static final array` instead of calling `CipherSuite.values` each time.
>>
>> Benchmark (cipherSuite) Mode Cnt Score Error Units
>> CipherSuiteBench.benchmarkCipherSuite TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op
>>
>>
>> 3. Using Hashmap for lookup instead of iterating through the array each time. (Method in this PR)
>>
>> Benchmark (cipherSuite) Mode Cnt Score Error Units
>> CipherSuiteBench.benchmarkCipherSuite TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op
>> CipherSuiteBench.benchmarkCipherSuite TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op
>>
>>
>> I have picked 4 cipher suite from the start of the list and are roughly 10 positions apart. I have opted to go with HashMap for name and id lookup as they provide a more consistent times and benchmarks are similar for the first few cipher suits in the enum as well.
>
> Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:
>
> Move to Immutable map and reduce map lookups
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 858:
> 856: private static final Map<Integer, CipherSuite> maps_id;
> 857: private static final Map<String, CipherSuite> maps_name;
> 858: private static final CipherSuite[] ciphers = CipherSuite.values();
Generally, Java follows camel-case syntax for names. It would be good to keep the coding style consistent and use instinctive names. I may use names like "cipherSuiteIds", "cipherSuiteNames" and "cipherSuites" for the above three fields.
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 920:
> 918:
> 919: static String nameOf(int id) {
> 920: String name = maps_id.get(id).name;
I guess get() may return null. It may be safe to check the return value of get():
CipherSuite cipherSuite = cipherSuiteIds.get(id);
if (cipherSuite != null) {
return cipherSuite.name;
}
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 931:
> 929: static Collection<CipherSuite> allowedCipherSuites() {
> 930: Collection<CipherSuite> cipherSuites = new LinkedList<>();
> 931: for (CipherSuite cs : ciphers) {
I may worthy of a static class field for the allowed cipher suites.
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 945:
> 943: static Collection<CipherSuite> defaultCipherSuites() {
> 944: Collection<CipherSuite> cipherSuites = new LinkedList<>();
> 945: for (CipherSuite cs : ciphers) {
I may worthy of a static class field for the default cipher suites.
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 977:
> 975:
> 976: boolean found = false;
> 977: for (CipherSuite cs : ciphers) {
If there is a static allowed cipher suite list, the logic here could be simplified to use the allowed cipher suite list. Then, the "ciphers" class filed may be not needed any longer.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4783
More information about the security-dev
mailing list