Hi Max,
Thanks for the comments, they are very helpful. I should have webrev.02
ready in a bit. Please find comments below in line.
On 5/1/2020 2:45 AM, Weijun Wang wrote:
ObjectIdentifier.java
---------------------
Have you thought about storing the ObjectIdentifier object somewhere?
ObjectIdentifier.of() creates a new object each time and the conversion of
string to byte[] might be a performance issue. We used to have a lot of
ObjectIdentifier objects in AlgorithmId but now we only have KnownOIDs.
The ObjectIdentifier objects are stored inside the oidTable map in
AlgorithmId class. I removed the static oid constants which aren't
referenced. The oids are retrieved from the oidTable when needed. With
the webrev, we are exchange oid construction with a map look up, at
least this is the intention.
I had a talk with Stuart and he has a suggestion that we can stuff all pre-calculated OID DER
encodings in a long byte array in a resource file, and in KnownOIDs each element has an
offset/length pair that point to its DER encoding. Also, whatever cache mechanism we use in the
future, I suggest making "new ObjectIdentifier(String)" private and keep
"ObjectIdentifier of(String)".
Done in webrev.02.
SecurityProviderConstants.java
------------------------------
public static List<String> alias(String ... aliases) {
return Arrays.asList(aliases);
}
Probably not necessary, you can simply call List.of(...) everywhere.
Done in webrev.02.
SunMSCAPI.java
--------------
Why not call getAliases() inside "new ProviderService" like in the other
providers? Same in UCrypto.
Ok, done in webrev.02.
AlgorithmId.java
----------------
algOID(String). You don't check "if (name.indexOf('.') != -1)" at the beginning anymore.
Is there an algorithm name containing "."?
Nope, it's more because the KnownOIDs enum can handle oid string lookup,
so I just need to remove the "OID." prefix if present.
It's a pity we have to collect OIDs from other providers. Maybe it should only
be necessary when we use that provider, for example, when encoding a signature,
we should ask the provider about the OID. I wish there were a
Signature::getAlgorithmId, but if not, maybe we can rename
Algorithm::alfOID(name) to Algorithm::alfOID(name, provider).
The existing OID collection code in AlgorithmId class serves as a safety
net when new OID alias mapping is added to provider but AlgorithmId
class is not also updated. After this KnownOIDs change, this safety net
may not be that necessary and for performance reason, the code would
skip JDK providers. However, for max backward compatibility, we may have
to keep it. I agree with your name->oid mapping can be provider-specific
and handling that would be somewhat sticky with current code. As for
including an extra provider argument, I am not sure if that'd work
across the board as the caller of AlgorithmId class may not have the
provider info. OID usage can be somewhat complicated as demonstrated by
this change.
Do you know a bad case if we don't collect those OIDs? It must be some
algorithm that is not in the Standard Names.
With existing regression tests, nothing breaks if we don't collect the
OID aliases.
Overall I think the change looks great, and we have a single place to store all
OIDs. The mapping among the OID string, KnownOIDs, and ObjectIdentifier could
be enhanced. Do you have a benchmark?
I have not done one. I can try something out once I finish addressing
your other comments.
Thanks,
Valerie
Thanks,
Max
On Apr 30, 2020, at 6:59 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
Here is the updated webrev
http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/
