[Pki-devel] [PATCH] 4 Replaced byte-to-char & char-to-byte converters.

[Andrew Wnuk]

On 12/21/2011 01:56 PM, Ade Lee wrote:
> On Wed, 2011-12-21 at 12:31 -0800, Andrew Wnuk wrote:
>> On 12/21/2011 11:23 AM, Ade Lee wrote:
>>>>> If there is an unacceptable degradation in performance, perhaps we could
>>>>> utilize some customized Java classes to perform these functions. As an
>>>>> alternative approach, as Endi alluded to, there is some ASN.1 code in
>>>>> Mozilla JSS, and speaking with Bob Relyea, we have been postulating on
>>>>> how much work would be involved to write JNI bindings via JSS to the
>>>>> ASN.1 encoders/decoders contained in NSS instead of moving this code to
>>>>> use java.nio.* classes. Theoretically, we may limit our performance
>>>>> issues in exchange for the extra work involved in making the effort to
>>>>> standardize on the NSS ASN.1 engine (although I don't know if this will
>>>>> resolve issues such as (3) noted above).
>>>> I checked the code, it looks like some of Mozilla's JSS code also relies
>>>> on the Charset API. JNI calls have some overhead too, so we need to know
>>>> how it's going to be used to avoid negative performance impact.
>>>>
>>> I agree the performance is a concern, and is something that we should
>>> keep in mind, but it is not clear that differences in performance in
>>> these classes will degrade the performance of the system.  The only way
>>> to determine where the system is really spending its time is by running
>>> it through a profiler and analyzing the results.  We may find that the
>>> server spends comparatively no time in this code.
>> I think "may" is a proper keyword here.  If someone, calls "Charset API"
>> a "bottleneck" that should be good enough warning to do some simple
>> testing to verify it.
>>
> Lets say we do a simple test and the encoding takes 3 ms instead of 1 ms
> to complete.  What then?  Do we reject the patch?  Do we embark on a
> process whereby we investigate custom code or look into JSS functions?
>
> No - we do not.  Why?  Because we have no idea whether this is even a
> bottleneck for our server.  Just because it was a bottleneck for someone
> else for some random app - does not mean it will be the same for us.  It
> may end up being a huge bottleneck - or it may be completely irrelevant.
> We do not know - and we will not know - until we do profiling.
>
> Without profiling information, we are optimizing in the dark, and we
> will very likely spend a lot of wasted time optimizing the wrong things.

I think the point is to check if this issue is real or not. This does 
not require any profiling or optimization just a simple test.

>
>>>
>>> On the other hand, we have a compelling reason to change these classes.
>>> The classes are deprecated - and will go away - and then everything will
>>> stop working.
>>>
>>> I suggest that we review this patch for correctness.
>> I think Matt's email was clear. This patch is fine but we had 3 concerns
>> which were listed in his email.
>>
>>>     Is the encoding
>>> correct?
>> There is no simple test to prove this or to replace years of
>> compatibility testing (case like (3) can be good example of this).
>>
>>>     Do the unit tests cover enough - or have the right inputs to
>>> validate the correctness of the conversion?  It seems like there were
>>> situations like (3) where perhaps more testing is required.
>> Agreed, there was always apparent reason for fixes like (3).
>>
>>>
>>> And we should schedule a performance testing and profiling task for next
>>> year.  We will always have the 8.x code as a benchmark.  If at that
>>> time, we find that these classes cause performance problems, we can
>>> always look into custom classes or maybe JSS code.
>>>
>>> It seems to me that talking about custom classes before having any
>>> profiling data is putting the cart before the horse.
>>>
>>> Ade
>>>
>>>
>