Hi Martin,
Thank you for considering to make the improvement in this update.
The use of synchronizedMap may hurt the performance. I may prefer to
use ConcurrentHashMap and its computeIfAbsent() method. (Read more,
this map may be not necessary)
621 interface CipherGenerator {
Could this interface be private? (Read more, this interface may be not
necessary)
592 if (rcg != null && wcg != null) {
Per the current implementation, checking read or write cipher generator
is sufficient. If you want to check the read one only, there is no need
to declare the CipherGenerator any more (line 621). It could be placed
in ReadCipherGenerator. (Read more, may not need to update
ReadCipherGenerator)
589-597 boolean supports(ProtocolVersion protocolVersion) {
Together with the CipherGenerator interface, the logic of this block is
actually about the transformation, not really about the cipher
generator. Is it simpler to check the transformation directly?
If we could check the transformation directly, it might be better to
have the check in the SSLCipher constructors, using immutable enum field.
Putting them together:
SSLCipher.java:
private SSLCipher(String transformation,
...
this.isAvailable =
- allowed && isUnlimited(keySize, transformation);
+ allowed && isUnlimited(keySize, transformation) &&
+ isAvailable(transformation);
}
+ private static boolean isAvailable(String transformation) {
+ if (!transformation.equals("NULL")) {
+ ... // check the instance
+ } else {
+ ...
+ }
+ }
SSLContextImpl.java:
382 if (!suite.supports(protocol) ||
-383 !suite.bulkCipher.supports(protocol)) {
+383 !suite.isAvailable()) {
BTW, the coding style looks a little bit uncommon to me (May not need
this block, anyway just for your consideration).
631 } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
632 }
633 cipherAlgorithms.put(algorithm, false);
Would you mind if not using blank catch block?
631 } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
+ cipherAlgorithms.put(algorithm, false);
+ if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+ ...
+ }
632 }
-633 cipherAlgorithms.put(algorithm, false);
Thanks & Regards,
Xuelei
On 5/27/2019 2:00 PM, Martin Balao wrote:
Hi Xuelei,
Thanks to you for raising these concerns and providing your feedback.
On 5/24/19 7:47 PM, Xuelei Fan wrote:
Good, I have no further comment for this update. Please go ahead.
I think there is a possible improvement by calling
Cipher.getInstance(algorithm) only one time for each transformation
algorithm. But may not worthy as the duplicated transformation
algorithm number is still small. I'm fine if you want to leave it as it
is.
Yes, I agree on both statements: there can be an improvement there but
it won't be significant. Here it's Webrev.02:
* http://cr.openjdk.java.net/~mbalao/webrevs/8223482/8223482.webrev.02/
At Webrev.02, Cipher.getInstance results are cached so we don't need to
create instances unnecessary.
Benchmarks do not show any significant performance increase nor decrease
compared to Webrev.01:
Benchmark (testMode) Mode Cnt
Score Error Units
SupportedCiphersuites.test_TLS12Communication FIPS thrpt 10
167.393 ± 35.659 ops/s
SupportedCiphersuites.test_TLS12Communication NON_FIPS thrpt 10
593.441 ± 110.044 ops/s
FIPS_with_8223482_webrev02.txt average: 396.86
NON_FIPS_with_8223482_webrev02.txt average: 888.05
Are you okay to go with Webrev.02?
Kind regards,
Martin.-
