[8] code review request for 7165807: Non optimized initialization of NSS crypto library leads to scalability issues

[Vincent Ryan]

Thanks for the review Valerie. Comments below.


On 26 Jun 2013, at 00:23, Valerie (Yu-Ching) Peng wrote:

> 
> <Secmod.java>
> - The private static native boolean nssInit(...) method should be removed? It seems obsolete by nssInitWithOptions(...) and I didn't see it being used anywhere. Same goes for the corresponding JNI method impl, i.e. Java_sun_security_pkcs11_Secmod_nssInit, in the j2secmod.c file.

I considered zapping nssInit() but I wanted to ensure I didn't break anything in the JDK 7 Update release.
I can remove it from the JDK 8 version.


> 
> <Config.java>
> - Particular reason to use different variable name, i.e. "nssUseOptimizeSpace" and property/option name "nssOptimizeSpace"? It seems inconsistent as most config variable name matches the property/option name. Can't we just use "nssOptimizeSpace"across board?

The previous name of the SunPKCS11 configuration flag was 'nssUseOptimizeSpace'.
I'll update all the internal references.


> 
> <j2secmod.c>
> - line 127, empty string is used instead of the passed configDir. I am not sure if this correct. Why not just pass the configDir to this call?

To be consistent with the NSS native code: it passes an empty string.


> 
> Lastly, is the "NSS_Initialize(...)" method always available for the supported NSS library versions, i.e. 3.7+? Is this a newer method meant to replace "NSS_Init(...)"?

It's available in addition to the NSS_Init* functions (not a replacement).



> Thanks,
> Valerie
> 
> On 06/19/13 10:38, Vincent Ryan wrote:
>> Thanks for the review. I've simplified the name of the NSS flag, updated the bug report and filed a doc bug,
>> as you suggest.
>> 
>> 
>> 
>> On 19 Jun 2013, at 18:21, Sean Mullan wrote:
>> 
>>> Looks good, just a couple of comments:
>>> 
>>> 1. I think the name "nssOptimizeSpace" is clearer. The "Use" part seems a bit odd in the property name.
>>> 
>>> 2. Add the appropriate noreg label to the bug.
>>> 
>>> 3. File a followup doc bug to document the attribute in the PKCS11 guide.
>>> 
>>> --Sean
>>> 
>>> On 06/19/2013 08:49 AM, Vincent Ryan wrote:
>>>> I've made some corrections to the native method that initializes NSS.
>>>> The new webrev is at:
>>>> 
>>>>   http://cr.openjdk.java.net/~vinnie/7165807/webrev.01
>>>> 
>>>> 
>>>> 
>>>> On 14 Jun 2013, at 18:38, Vincent Ryan wrote:
>>>> 
>>>>> Please review the following fix:
>>>>> 
>>>>> http://cr.openjdk.java.net/~vinnie/7165807/webrev.00/
>>>>> http://bugs.sun.com/view_bug.do?bug_id=7165807
>>>>> 
>>>>> NSS may be initialized to favour performance or to favour memory footprint.
>>>>> This fix introduces a new configuration flag to allow Java applications to choose.
>>>>> By default, NSS will be initialized for performance.
>>>>> 
>>>>> Thanks.
>>>>> 
>