[PATCH] 4 Replaced byte-to-char & char-to-byte converters.
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 -- Endi S. Dewata
On 12/13/2011 12:47 PM, Endi Sukma Dewata wrote:
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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Nicely done, I've given it a visual quick look over. A minor nit is that you shouldn't use import java.util.*; but instead list the elements individually. I would like to see this split into two patches, one that moves the files into the proper package and a second that changes to the nondeprecated API. If you have it that way in your Git repo, please post it, so I can see what is changed in the moved files. If not, it isn't that hard to set up to do the diff by hand.
On 12/13/2011 12:18 PM, Adam Young wrote:
On 12/13/2011 12:47 PM, Endi Sukma Dewata wrote:
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
Nicely done, I've given it a visual quick look over. A minor nit is that you shouldn't use
import java.util.*; but instead list the elements individually.
OK, that can be changed.
I would like to see this split into two patches, one that moves the files into the proper package and a second that changes to the nondeprecated API. If you have it that way in your Git repo, please post it, so I can see what is changed in the moved files. If not, it isn't that hard to set up to do the diff by hand.
The package isn't changed, I'm only renaming the files according to the class hierarchy. It probably wouldn't make sense to have an IA5CharsetDecoder inheriting from ByteToCharConverter, but I can redo the patch to do that if you want, it's not difficult. -- Endi S. Dewata
On 12/13/2011 09:47 AM, Endi Sukma Dewata wrote:
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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Hopefully this patch was extremely well tested since it touches ASN.1 encoding.
On 12/13/2011 02:03 PM, Andrew Wnuk wrote:
On 12/13/2011 09:47 AM, Endi Sukma Dewata wrote:
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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Hopefully this patch was extremely well tested since it touches ASN.1 encoding.
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel Do you have a specific set of tests that we should be running?
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/ -- Endi S. Dewata
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@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-char... * 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
If the only questions are those of performance, than it is a non issue for now. On 12/20/2011 09:19 PM, Matthew Harmsen wrote:
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@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-char... * http://www.theserverside.com/discussions/thread.tss?thread_id=61270
The article also states that the issue is being delat with in Java 7. We are standardizing on Java 7 for this release, so it should not be an issue.
(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
I am not worried about performance at this point, it is way too premature. I am only concerned with correctness. The deprecated classes are going away, and we need an alternative to them. We will have a deliberate performance and tuning iteration once the code base has been restructured. If we find at that stage that there are still performance issues, we can look into writing a custom tuned solution, but based on overall system performance.
On 12/20/2011 06:43 PM, Adam Young wrote:
If the only questions are those of performance, than it is a non issue for now.
On 12/20/2011 09:19 PM, Matthew Harmsen wrote:
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@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-char... * http://www.theserverside.com/discussions/thread.tss?thread_id=61270
The article also states that the issue is being delat with in Java 7.
and this is what we need to verify.
We are standardizing on Java 7 for this release, so it should not be an issue.
(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
I am not worried about performance at this point,
Performance is not easily achievable and ignoring it is not the best practice for developing servers.
it is way too premature. I am only concerned with correctness. The deprecated classes are going away, and we need an alternative to them. We will have a deliberate performance and tuning iteration once the code base has been restructured. If we find at that stage that there are still performance issues, we can look into writing a custom tuned solution, but based on overall system performance.
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-char...
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.ht...) 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
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. 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. Is the encoding correct? 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. 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 12/21/2011 02:23 PM, Ade Lee wrote:
It seems to me that talking about custom classes before having any profiling data is putting the cart before the horse.
+1 Correctness comes first, do not optimize without profiling data (assumptions about performance are usually incorrect), then only optimize based on profiling data and only for cases that matter, do not give up code simplicity and robustness unless there is a real valid case that demands it. -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
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 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
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
Hi, I've rebased the patches. They should be applied after patch #8 and #9. Please take a look at #8 and see if it's sufficient to verify the correctness. 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. -- Endi S. Dewata
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. -- Endi S. Dewata
On 01/06/2012 04:11 PM, Endi Sukma Dewata wrote:
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. I agree that the numbers don't show anything that we should be concerned with at this point. If your tests from patch #8 prove encoding/decoding correctness, I think we should move forward. We should file a trac ticket for general performance profiling and target it for a future milestone (likely a few months from now).
-NGK
On 1/6/2012 6:21 PM, Nathan Kinder wrote:
I agree that the numbers don't show anything that we should be concerned with at this point. If your tests from patch #8 prove encoding/decoding correctness, I think we should move forward. We should file a trac ticket for general performance profiling and target it for a future milestone (likely a few months from now).
I ran a smoke test, everything seems to be working fine. ACKed by Ade, pushed to master. -- Endi S. Dewata
participants (7)
-
Adam Young -
Ade Lee -
Andrew Wnuk -
Endi Sukma Dewata -
John Dennis -
Matthew Harmsen -
Nathan Kinder