One more thing:

In KnownOIDs.java, I found these 2 lines:

    PKIX_KP_BASE("1.3.6.1.5.5.7.3."),
    PKIX_OCSP_BASE("1.3.6.1.5.5.7.48."),

IMHO, they should not belong here, at least, we should remove the dot at the 
end and make them real OIDs.

I was testing the ObjectIdentifier generation and notice these two.

Thanks,
Max


> On May 1, 2020, at 5:45 PM, Weijun Wang <weijun.w...@oracle.com> 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.
> 
> 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)".
> 
> SecurityProviderConstants.java
> ------------------------------
> 
>    public static List<String> alias(String ... aliases) {
>        return Arrays.asList(aliases);
>    }
> 
> Probably not necessary, you can simply call List.of(...) everywhere.
> 
> SunMSCAPI.java
> --------------
> 
> Why not call getAliases() inside "new ProviderService" like in the other 
> providers? Same in UCrypto.
> 
> AlgorithmId.java
> ----------------
> 
> algOID(String). You don't check "if (name.indexOf('.') != -1)" at the 
> beginning anymore. Is there an algorithm name containing "."?
> 
> 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).
> 
> 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.
> 
> 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?
> 
> 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/
> 

Reply via email to