RFR: 8270317: Large Allocation in CipherSuite [v2]

[Xue-Lei Andrew Fan]

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