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

[Endi Sukma Dewata]

On 12/20/2011 8:19 PM, Matthew Harmsen wrote:
> 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

The patch #4-2 a&b are addressing this issue by replacing the converters 
with charset encoders/decoders. I suppose when we use the 
java.nio.Charset API we will indirectly use the sun.nio.cs library.

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

There isn't enough details to know what the actual problem is. Also the 
person asking was using Java 5, so it might not apply anymore in Java 7.

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

I think the above posting and the Java Code Geeks article 
(http://www.javacodegeeks.com/2010/11/java-best-practices-char-to-byte-and.html) 
are discussing about converting chars to bytes for ultra high 
performanace data serialization, not for encoding, so it doesn't apply 
to our case. Here's what the article says:

   Keep in mind that we are not converting between character encodings.
   For converting between character encodings you should stick with the
   “classic” approaches using either the “String.getBytes(charsetName)”
   or the NIO framework methods and utilities.

While performance is a valid concern, I don't think the existing code 
has been much optimized for high performance. For example, the getCBC() 
and getBCC() do not cache the converter instances they create.

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

Do we know what problems are being fixed? I see one comment in the code 
about issue #359010 which will skip non-printable chars instead of 
reporting it. I suppose this is because there's no mechanism to ignore 
invalid input in sun.io.* packages. The Charset API provides an option 
to ignore invalid input, but we need to make sure we are using it 
correctly. Are there specific cases where we want to skip non-printable 
chars instead of throwing an exception?

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

I think we need to know the use cases that require high performance. 
Optimizing the converter alone might not be enough or worth the effort. 
Depending on the use case, there could be worse bottlenecks in other 
parts of the application.

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

In order to address issue #3 we need to have a good test coverage. To 
verify the correctness, are there anything else that we need to include 
in the test case?

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

-- 
Endi S. Dewata