On Wed, 20 Oct 2021 02:45:07 GMT, Anthony Scarpino <ascarp...@openjdk.org>
wrote:
>> Sean Mullan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> - Changed names of AlgorithmDecomposer.canonicalName and
>> decomposeOneHashName
>> methods.
>> - Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES
>> Map instead of hardcoding algorithm names.
>> - Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so
>> that
>> constraints on the key algorithm and size are checked in the check()
>> method if
>> the constraints are an instanceof DisabledAlgorithmConstraints.
>
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
> 106:
>
>> 104: // "SHA-256" and "SHA256" to make the right constraint checking.
>> 105:
>> 106: for (Map.Entry<String, String> e :
>> DECOMPOSED_DIGEST_NAMES.entrySet()) {
>
> If you're going to change this code, you can save me a PR if you surround
> this by "if (algorithm.contains("SHA") { ... }"
> Its a perf change to eliminate the unnecessary map lookups when SHA isn't in
> the algorithm string
That's a fine suggestion, although I'll note that your suggested perf
improvement also applies to the previous code which did not check the algorithm
parameter first to see if it contained `SHA`.
Also, another small perf imp: I realized below that in the loop, if the first
`if` block gets executed, then the 2nd `if` block will always be false, so I
changed it to an if/else.
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
> 158:
>
>> 156: Set<String> elements = decomposeImpl(algorithm);
>> 157:
>> 158: for (Map.Entry<String, String> e :
>> DECOMPOSED_DIGEST_NAMES.entrySet()) {
>
> Same "if (algorithm.contains("SHA") { ... }" comment as above
Ok. Let's hope a new algorithm with "SHA" in the words never gets defined :)
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
> 159:
>
>> 157:
>> 158: for (Map.Entry<String, String> e :
>> DECOMPOSED_DIGEST_NAMES.entrySet()) {
>> 159: hasLoop(elements, e.getKey(), e.getValue());
>
> I think it's worth merging the contents of hasLoop() into this for loop.
> This is the only method calling hasLoop() and the extra method calls are not
> useful. Your addition DECOMPOSED_DIGEST_NAMES makes a merger a more
> reasonable solution now than before.
Ok. I noticed that this method is different than `decompose` in that it only
adds the decomposed `SHA` name to `elements`, and removes the standard `SHA`
name from `elements` if it exists, whereas `decompose` adds both. Do you think
we should change `decompose` to use the same technique?
-------------
PR: https://git.openjdk.java.net/jdk/pull/5928
