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

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.

>> >> 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 >> >>