The byte-to-char and char-to-byte converters have been replaced with a set of charsets, each having its own decoders and encoders. Ticket #3 _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

The byte-to-char and char-to-byte converters have been replaced with a set of charsets, each having its own decoders and encoders. Ticket #3 _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

On 12/13/2011 02:03 PM, Andrew Wnuk wrote: > Hopefully this patch was extremely well tested since it touches ASN.1 > encoding. Do you have a specific set of tests that we should be running?

On 12/16/11 17:24, Endi Sukma Dewata wrote: > On 12/13/2011 4:25 PM, Adam Young wrote: >> On 12/13/2011 02:03 PM, Andrew Wnuk wrote: >>> Hopefully this patch was extremely well tested since it touches ASN.1 >>> encoding. >> Do you have a specific set of tests that we should be running? > > I just posted patch #5 which contains some unit tests for the > converters. I've also updated and split patch #4 into #4-2a (renaming > the files) and #4-2b (replacing the implementation). > > Patch #5 should be applied before #4-2a/b to make sure there's no > regression. > > Mozilla JSS library also has some ASN.1 code, should we use them to > implement the converters? > > http://mxr.mozilla.org/mozilla/source/security/jss/org/mozilla/jss/asn1/ > > > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel Endi, Andrew and I have been looking at these changes, and we have the following concerns: (1) First of all, we looked up the bug where some of the sun.io.* classes were deprecated (which mentions java.nio.charset and sun.nio.cs): * http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4948149 (2) There appears to be considerable concern on performance degradation when using "Charset": * http://stackoverflow.com/questions/2098137/fast-alternative-to-java-nio-c... * http://www.theserverside.com/discussions/thread.tss?thread_id=61270

(3) Additionally, there have been alterations in the CS code in the past to fix problems encountered with the sun.io.* classes: * pki/base/util/src/netscape/security/util/ByteToCharPrintable.java Although we have not gotten to the unit tests, we suspect that these will be great to have regardless of the direction that is decided upon. However, due to our concerns regarding performance, could we have some tests added (unit or a test tool) which obtain performance results for the existing methods versus the proposed newer methods? If no discernable performance issues are encountered, most of these changes appear to be acceptable -- the questionable one being the replacing of the code that addressed issue (3) above. 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). -- Matt

Andrew and I have been looking at these changes, and we have the following concerns: (1) First of all, we looked up the bug where some of the sun.io.* classes were deprecated (which mentions java.nio.charset and sun.nio.cs): * http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4948149

(2) There appears to be considerable concern on performance degradation when using "Charset": * http://stackoverflow.com/questions/2098137/fast-alternative-to-java-nio-c...

* http://www.theserverside.com/discussions/thread.tss?thread_id=61270

(3) Additionally, there have been alterations in the CS code in the past to fix problems encountered with the sun.io.* classes: * pki/base/util/src/netscape/security/util/ByteToCharPrintable.java

Although we have not gotten to the unit tests, we suspect that these will be great to have regardless of the direction that is decided upon. However, due to our concerns regarding performance, could we have some tests added (unit or a test tool) which obtain performance results for the existing methods versus the proposed newer methods?

If no discernable performance issues are encountered, most of these changes appear to be acceptable -- the questionable one being the replacing of the code that addressed issue (3) above.

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

It seems to me that talking about custom classes before having any profiling data is putting the cart before the horse.

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.

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

On 1/5/2012 3:00 PM, Endi Sukma Dewata wrote: > About the performance, I compared the results of #9 before and after > conversion to Charset, there are some differences but I don't think the > difference is significant. To clarify, in patch #9 I wrote simple tests for measuring the encoding and decoding times for each converter (identified by the tag) to process a set of chars/bytes 10k times. for (int i=0; i<10000; i++) encode(tag, chars); for (int i=0; i<10000; i++) decode(tag, bytes); Each test is done twice, one using Mozilla JSS library and another using our internal library which is based on sun.io before the upgrade and java.nio (Charset) after the upgrade. Below is the result. The 2 numbers in each category correspond to the execution times before and after the upgrade. BMP String Encoding time: - JSS : 210 ms 212 ms - Internal: 281 ms 319 ms BMP String Decoding time: - JSS : 186 ms 196 ms - Internal: 212 ms 149 ms IA5 String Encoding time: - JSS : 42 ms 101 ms - Internal: 46 ms 110 ms IA5 String Decoding time: - JSS : 93 ms 118 ms - Internal: 84 ms 37 ms Printable String Encoding time: - JSS : 26 ms 113 ms - Internal: 48 ms 86 ms Printable String Decoding time: - JSS : 37 ms 49 ms - Internal: 40 ms 38 ms Teletex String Encoding time: - JSS : 18 ms 36 ms - Internal: 43 ms 103 ms Teletex String Decoding time: - JSS : 16 ms 16 ms - Internal: 23 ms 32 ms Universal String Encoding time: - JSS : 74 ms 71 ms - Internal: 30 ms 49 ms Universal String Decoding time: - JSS : 80 ms 76 ms - Internal: 42 ms 18 ms UTF-8 String Encoding time: - JSS : 13 ms 31 ms - Internal: 28 ms 59 ms UTF-8 String Decoding time: - JSS : 37 ms 51 ms - Internal: 46 ms 52 ms The JSS tests are supposed to produce similar numbers before and after the upgrade since they are executing the same code, but that's not always the case, see IA5 encoding and printable encoding times. This is probably due to the changing nature of machine load at the time of the execution. If we look at the internal numbers, some go up and some go down. We cannot really say whether it's an improvement or degradation since the JSS numbers that we use as a baseline aren't always stable. Also since different converter uses different code, there isn't a single number that can describe the overall behavior. So all I can conclude is that the upgrade to the internal library does not show any significant performance degradation that we should be concerned about at this point. If we want to get real statistical numbers (e.g. % improvement over the old library or JSS) we will have to define the test scope, generate better test data, and execute the tests systematically with varying loads. I'm not sure if we want to do that at this stage.