RFR [9]: 8050142: Optimize java.util.Formatter
Hi, please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details. webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142 Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks Thanks! /Claes
Hi Claes, in the method Formatter$FormatSpecifier#justify(String) you can pre-calculate the capacity of the StringBuilder to avoid array copying, e.g. instead of 2931 StringBuilder sb = new StringBuilder(); use this one: 2931 StringBuilder sb = new StringBuilder(s.length() + sp); And in the method Formatter$FormatSpecifier#justify(StringBuilder) you can avoid creation of the StringBuilder object in the line 2956, e.g. instead of: 2956 StringBuilder tmp = new StringBuilder(sp); 2957 for (int i = 0; i < sp; i++) { 2958 tmp.append(' '); 2959 } 2960 sb.insert(0, tmp); you can write this: 2956 char[] tmp = new char[sp]; 2957 Arrays.fill(tmp, ' '); 2958 sb.insert(0, tmp); It's not a big improvement but maybe you can change the line 3781 exp.append("0").append(len - 1); to use the character-based API to append a single character. Best regards, Andrej Golovnin On Mon, Jul 14, 2014 at 12:07 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi, good suggestions, I'll incorporate them. /Claes On 2014-07-14 13:08, Andrej Golovnin wrote:
Hi Claes,
in the method Formatter$FormatSpecifier#justify(String) you can pre-calculate the capacity of the StringBuilder to avoid array copying, e.g. instead of
2931 StringBuilder sb = new StringBuilder();
use this one:
2931 StringBuilder sb = new StringBuilder(s.length() + sp);
And in the method Formatter$FormatSpecifier#justify(StringBuilder) you can avoid creation of the StringBuilder object in the line 2956, e.g. instead of:
2956 StringBuilder tmp = new StringBuilder(sp); 2957 for (int i = 0; i < sp; i++) { 2958 tmp.append(' '); 2959 } 2960 sb.insert(0, tmp);
you can write this:
2956 char[] tmp = new char[sp]; 2957 Arrays.fill(tmp, ' '); 2958 sb.insert(0, tmp);
It's not a big improvement but maybe you can change the line
3781 exp.append("0").append(len - 1);
to use the character-based API to append a single character.
Best regards, Andrej Golovnin
On Mon, Jul 14, 2014 at 12:07 PM, Claes Redestad <claes.redestad@oracle.com <mailto:claes.redestad@oracle.com>> wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 <http://cr.openjdk.java.net/%7Eredestad/8050142/webrev.0> bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes, Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether: http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ Regards, Peter
Hi Peter, On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions. /Claes
Regards, Peter
On 07/14/2014 01:29 PM, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Hi Claes, If you're in a really hyper-optimizing mood, you could also eliminate construction of ArrayList in parse(): http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.02/ Regards, Peter
Hi Peter, On 2014-07-14 14:05, Peter Levart wrote:
On 07/14/2014 01:29 PM, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Hi Claes,
If you're in a really hyper-optimizing mood, you could also eliminate construction of ArrayList in parse():
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.02/
Have you measured this to have a real effect? I'm not convinced it would matter enough to warrant the extra complexity of turning FormatString into a linked list. I feel this patch is already a bit saturated with different small optimizations - maybe should deal with this in a new RFE? /Claes
Regards, Peter
On 07/14/2014 02:17 PM, Claes Redestad wrote:
If you're in a really hyper-optimizing mood, you could also eliminate construction of ArrayList in parse():
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.02/
Have you measured this to have a real effect? I'm not convinced it would matter enough to warrant the extra complexity of turning FormatString into a linked list. I feel this patch is already a bit saturated with different small optimizations - maybe should deal with this in a new RFE?
/Claes
You may be right. I agree that it's already pretty saturated patch. Maybe later if benchmarks show it's worth it. Regards, Peter
Hi again, updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1 changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller /Claes On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); maybe 2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); } Sincerely yours, Ivan On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Yes, might be worth addressing just for correctness/readability. /Claes On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Hi, final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2 Thanks in advance for reviews. I also need a volunteer to sponsor this. :-) /Claes On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Claes, Maybe change (or not): -throw new UnknownFormatConversionException(String.valueOf(c)); +throw new UnknownFormatConversionException(String.valueOf(conv)); I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue. Jason ----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote: > Hi, > > please review this patch which optimizes away some allocations > from java.util.Formatter and achieve 1.1-1.3x speedups of micros > targetting String.format. See bug for more details. > > webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 > bug: https://bugs.openjdk.java.net/browse/JDK-8050142 > > Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), > SPECjbb2013 and microbenchmarks > > Thanks! > > /Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes. New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3 Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466 /Claes On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote: > On 07/14/2014 12:07 PM, Claes Redestad wrote: >> Hi, >> >> please review this patch which optimizes away some allocations >> from java.util.Formatter and achieve 1.1-1.3x speedups of micros >> targetting String.format. See bug for more details. >> >> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >> >> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >> SPECjbb2013 and microbenchmarks >> >> Thanks! >> >> /Claes > Hi Claes, > > Since justify() result is always appended to the resulting > Appendable, you could merge the functionality and eliminate > constructing intermediary StringBuilder altogether: > > http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ > Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes > Regards, Peter
Hi, Claes I've looked over the latest changes, and they look good to me. I can sponsor this. Note that we need approval from a Reviewer. One small nitpick from me: 2914: If the text is left-justified, then aren't we padding on the right? I would call this "padRight". Thanks, -Brent On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes.
New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3
Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466
/Claes
On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote: > Hi Peter, > > On 2014-07-14 13:25, Peter Levart wrote: >> On 07/14/2014 12:07 PM, Claes Redestad wrote: >>> Hi, >>> >>> please review this patch which optimizes away some allocations >>> from java.util.Formatter and achieve 1.1-1.3x speedups of micros >>> targetting String.format. See bug for more details. >>> >>> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >>> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >>> >>> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >>> SPECjbb2013 and microbenchmarks >>> >>> Thanks! >>> >>> /Claes >> Hi Claes, >> >> Since justify() result is always appended to the resulting >> Appendable, you could merge the functionality and eliminate >> constructing intermediary StringBuilder altogether: >> >> http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ >> > Looks good, especially eliminating the need for two different > append methods. I'll update based on this and other suggestions. > > /Claes >> Regards, Peter
I am reviewer and they look good to me too. ~30% better performance for random indata- Do you pass the JCK? If so you have my blessing. /M On 16 Sep 2014, at 01:52, Brent Christian <brent.christian@oracle.com> wrote:
Hi, Claes
I've looked over the latest changes, and they look good to me. I can sponsor this. Note that we need approval from a Reviewer.
One small nitpick from me: 2914: If the text is left-justified, then aren't we padding on the right? I would call this "padRight".
Thanks, -Brent
On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes.
New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3
Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466
/Claes
On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:
A very minor one: 2704 if (Character.isUpperCase(conv)) 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv);
maybe
2704 if (Character.isUpperCase(conv)) { 2705 f.add(Flags.UPPERCASE); 2706 c = Character.toLowerCase(conv); }
Sincerely yours, Ivan
On 14.07.2014 16:23, Claes Redestad wrote: > Hi again, > > updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1 > > changes: > - specify capacity on line 2931 as suggested by Andrej Golovnin > - exp.append("0") -> exp.append('0') on line 3781 > - merged append+justify into appendJustified as suggested by Peter > Levart > - replaced the reoccuring pattern of appending a number of zeros > into a call to trailingZeros > > performance difference seemingly at noise levels in micros, but > bonus to readability and Formatter*.class-files are now a total of > 246 bytes smaller > > /Claes > > On 2014-07-14 13:29, Claes Redestad wrote: >> Hi Peter, >> >> On 2014-07-14 13:25, Peter Levart wrote: >>> On 07/14/2014 12:07 PM, Claes Redestad wrote: >>>> Hi, >>>> >>>> please review this patch which optimizes away some allocations >>>> from java.util.Formatter and achieve 1.1-1.3x speedups of micros >>>> targetting String.format. See bug for more details. >>>> >>>> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >>>> >>>> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >>>> SPECjbb2013 and microbenchmarks >>>> >>>> Thanks! >>>> >>>> /Claes >>> Hi Claes, >>> >>> Since justify() result is always appended to the resulting >>> Appendable, you could merge the functionality and eliminate >>> constructing intermediary StringBuilder altogether: >>> >>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ >>> >> Looks good, especially eliminating the need for two different >> append methods. I'll update based on this and other suggestions. >> >> /Claes >>> Regards, Peter > >
Brent, Marcus, thank you for reviewing! JCK 9 tests for api/java_util api/java_lang either pass or fail with or without this patch on my machine. Also reran relevant jtreg tests (jdk/test/java/util/Formatter jdk/test/java/util/Formattable jdk/test/java/lang/String jdk/test/java/lang/System) Ok to fix nits offline without posting new webrev? /Claes On 09/16/2014 11:34 AM, Marcus Lagergren wrote:
I am reviewer and they look good to me too. ~30% better performance for random indata- Do you pass the JCK? If so you have my blessing.
/M
On 16 Sep 2014, at 01:52, Brent Christian <brent.christian@oracle.com> wrote:
Hi, Claes
I've looked over the latest changes, and they look good to me. I can sponsor this. Note that we need approval from a Reviewer.
One small nitpick from me: 2914: If the text is left-justified, then aren't we padding on the right? I would call this "padRight".
Thanks, -Brent
On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes.
New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3
Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466
/Claes
On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote:
Yes, might be worth addressing just for correctness/readability.
/Claes
On 07/14/2014 03:02 PM, Ivan Gerasimov wrote: > A very minor one: > 2704 if (Character.isUpperCase(conv)) > 2705 f.add(Flags.UPPERCASE); > 2706 c = Character.toLowerCase(conv); > > maybe > > 2704 if (Character.isUpperCase(conv)) { > 2705 f.add(Flags.UPPERCASE); > 2706 c = Character.toLowerCase(conv); > } > > Sincerely yours, > Ivan > > > On 14.07.2014 16:23, Claes Redestad wrote: >> Hi again, >> >> updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1 >> >> changes: >> - specify capacity on line 2931 as suggested by Andrej Golovnin >> - exp.append("0") -> exp.append('0') on line 3781 >> - merged append+justify into appendJustified as suggested by Peter >> Levart >> - replaced the reoccuring pattern of appending a number of zeros >> into a call to trailingZeros >> >> performance difference seemingly at noise levels in micros, but >> bonus to readability and Formatter*.class-files are now a total of >> 246 bytes smaller >> >> /Claes >> >> On 2014-07-14 13:29, Claes Redestad wrote: >>> Hi Peter, >>> >>> On 2014-07-14 13:25, Peter Levart wrote: >>>> On 07/14/2014 12:07 PM, Claes Redestad wrote: >>>>> Hi, >>>>> >>>>> please review this patch which optimizes away some allocations >>>>> from java.util.Formatter and achieve 1.1-1.3x speedups of micros >>>>> targetting String.format. See bug for more details. >>>>> >>>>> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >>>>> >>>>> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >>>>> SPECjbb2013 and microbenchmarks >>>>> >>>>> Thanks! >>>>> >>>>> /Claes >>>> Hi Claes, >>>> >>>> Since justify() result is always appended to the resulting >>>> Appendable, you could merge the functionality and eliminate >>>> constructing intermediary StringBuilder altogether: >>>> >>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ >>>> >>> Looks good, especially eliminating the need for two different >>> append methods. I'll update based on this and other suggestions. >>> >>> /Claes >>>> Regards, Peter >>
Hi, Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting): http://cr.openjdk.java.net/~redestad/8050142/webrev.07/ This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h... /Claes On 09/16/2014 03:15 PM, Claes Redestad wrote:
Brent, Marcus,
thank you for reviewing!
JCK 9 tests for api/java_util api/java_lang either pass or fail with or without this patch on my machine.
Also reran relevant jtreg tests (jdk/test/java/util/Formatter jdk/test/java/util/Formattable jdk/test/java/lang/String jdk/test/java/lang/System)
Ok to fix nits offline without posting new webrev?
/Claes
On 09/16/2014 11:34 AM, Marcus Lagergren wrote:
I am reviewer and they look good to me too. ~30% better performance for random indata- Do you pass the JCK? If so you have my blessing.
/M
On 16 Sep 2014, at 01:52, Brent Christian <brent.christian@oracle.com> wrote:
Hi, Claes
I've looked over the latest changes, and they look good to me. I can sponsor this. Note that we need approval from a Reviewer.
One small nitpick from me: 2914: If the text is left-justified, then aren't we padding on the right? I would call this "padRight".
Thanks, -Brent
On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes.
New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3
Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466
/Claes
On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
----------------------------------------
Date: Mon, 14 Jul 2014 17:40:41 +0200 From: claes.redestad@oracle.com To: core-libs-dev@openjdk.java.net Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter
Hi,
final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2
Thanks in advance for reviews. I also need a volunteer to sponsor this. :-)
/Claes
On 07/14/2014 04:21 PM, Claes Redestad wrote: > Yes, might be worth addressing just for correctness/readability. > > /Claes > > On 07/14/2014 03:02 PM, Ivan Gerasimov wrote: >> A very minor one: >> 2704 if (Character.isUpperCase(conv)) >> 2705 f.add(Flags.UPPERCASE); >> 2706 c = Character.toLowerCase(conv); >> >> maybe >> >> 2704 if (Character.isUpperCase(conv)) { >> 2705 f.add(Flags.UPPERCASE); >> 2706 c = Character.toLowerCase(conv); >> } >> >> Sincerely yours, >> Ivan >> >> >> On 14.07.2014 16:23, Claes Redestad wrote: >>> Hi again, >>> >>> updated webrev: >>> http://cr.openjdk.java.net/~redestad/8050142/webrev.1 >>> >>> changes: >>> - specify capacity on line 2931 as suggested by Andrej Golovnin >>> - exp.append("0") -> exp.append('0') on line 3781 >>> - merged append+justify into appendJustified as suggested by >>> Peter >>> Levart >>> - replaced the reoccuring pattern of appending a number of zeros >>> into a call to trailingZeros >>> >>> performance difference seemingly at noise levels in micros, but >>> bonus to readability and Formatter*.class-files are now a >>> total of >>> 246 bytes smaller >>> >>> /Claes >>> >>> On 2014-07-14 13:29, Claes Redestad wrote: >>>> Hi Peter, >>>> >>>> On 2014-07-14 13:25, Peter Levart wrote: >>>>> On 07/14/2014 12:07 PM, Claes Redestad wrote: >>>>>> Hi, >>>>>> >>>>>> please review this patch which optimizes away some allocations >>>>>> from java.util.Formatter and achieve 1.1-1.3x speedups of >>>>>> micros >>>>>> targetting String.format. See bug for more details. >>>>>> >>>>>> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >>>>>> >>>>>> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >>>>>> SPECjbb2013 and microbenchmarks >>>>>> >>>>>> Thanks! >>>>>> >>>>>> /Claes >>>>> Hi Claes, >>>>> >>>>> Since justify() result is always appended to the resulting >>>>> Appendable, you could merge the functionality and eliminate >>>>> constructing intermediary StringBuilder altogether: >>>>> >>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ >>>>> >>>>> >>>> Looks good, especially eliminating the need for two different >>>> append methods. I'll update based on this and other suggestions. >>>> >>>> /Claes >>>>> Regards, Peter >>>
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes, Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp. Thanks! -Sherman
/Claes
On 09/16/2014 03:15 PM, Claes Redestad wrote:
Brent, Marcus,
thank you for reviewing!
JCK 9 tests for api/java_util api/java_lang either pass or fail with or without this patch on my machine.
Also reran relevant jtreg tests (jdk/test/java/util/Formatter jdk/test/java/util/Formattable jdk/test/java/lang/String jdk/test/java/lang/System)
Ok to fix nits offline without posting new webrev?
/Claes
On 09/16/2014 11:34 AM, Marcus Lagergren wrote:
I am reviewer and they look good to me too. ~30% better performance for random indata- Do you pass the JCK? If so you have my blessing.
/M
On 16 Sep 2014, at 01:52, Brent Christian <brent.christian@oracle.com> wrote:
Hi, Claes
I've looked over the latest changes, and they look good to me. I can sponsor this. Note that we need approval from a Reviewer.
One small nitpick from me: 2914: If the text is left-justified, then aren't we padding on the right? I would call this "padRight".
Thanks, -Brent
On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv instead, but I think I need to be conservative now and will revert the name changes.
New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3
Changing usage of given locale or any semantic change is really out-of-scope here. There seems to be a few ancient bugs hanging around which maybe someone should take a look at, e.g., https://bugs.openjdk.java.net/browse/JDK-5063466
/Claes
On 2014-07-14 20:05, Jason Mehrens wrote:
Claes,
Maybe change (or not):
-throw new UnknownFormatConversionException(String.valueOf(c));
+throw new UnknownFormatConversionException(String.valueOf(conv));
I haven't examined it too deeply but it seems odd that some of those print methods don't use the given locale when converting case. That is probably not a change for this issue.
Jason
---------------------------------------- > Date: Mon, 14 Jul 2014 17:40:41 +0200 > From: claes.redestad@oracle.com > To: core-libs-dev@openjdk.java.net > Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter > > Hi, > > final(?) webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.2 > > Thanks in advance for reviews. I also need a volunteer to sponsor > this. :-) > > /Claes > > On 07/14/2014 04:21 PM, Claes Redestad wrote: >> Yes, might be worth addressing just for correctness/readability. >> >> /Claes >> >> On 07/14/2014 03:02 PM, Ivan Gerasimov wrote: >>> A very minor one: >>> 2704 if (Character.isUpperCase(conv)) >>> 2705 f.add(Flags.UPPERCASE); >>> 2706 c = Character.toLowerCase(conv); >>> >>> maybe >>> >>> 2704 if (Character.isUpperCase(conv)) { >>> 2705 f.add(Flags.UPPERCASE); >>> 2706 c = Character.toLowerCase(conv); >>> } >>> >>> Sincerely yours, >>> Ivan >>> >>> >>> On 14.07.2014 16:23, Claes Redestad wrote: >>>> Hi again, >>>> >>>> updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1 >>>> >>>> changes: >>>> - specify capacity on line 2931 as suggested by Andrej Golovnin >>>> - exp.append("0") -> exp.append('0') on line 3781 >>>> - merged append+justify into appendJustified as suggested by Peter >>>> Levart >>>> - replaced the reoccuring pattern of appending a number of zeros >>>> into a call to trailingZeros >>>> >>>> performance difference seemingly at noise levels in micros, but >>>> bonus to readability and Formatter*.class-files are now a total of >>>> 246 bytes smaller >>>> >>>> /Claes >>>> >>>> On 2014-07-14 13:29, Claes Redestad wrote: >>>>> Hi Peter, >>>>> >>>>> On 2014-07-14 13:25, Peter Levart wrote: >>>>>> On 07/14/2014 12:07 PM, Claes Redestad wrote: >>>>>>> Hi, >>>>>>> >>>>>>> please review this patch which optimizes away some allocations >>>>>>> from java.util.Formatter and achieve 1.1-1.3x speedups of micros >>>>>>> targetting String.format. See bug for more details. >>>>>>> >>>>>>> webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 >>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8050142 >>>>>>> >>>>>>> Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), >>>>>>> SPECjbb2013 and microbenchmarks >>>>>>> >>>>>>> Thanks! >>>>>>> >>>>>>> /Claes >>>>>> Hi Claes, >>>>>> >>>>>> Since justify() result is always appended to the resulting >>>>>> Appendable, you could merge the functionality and eliminate >>>>>> constructing intermediary StringBuilder altogether: >>>>>> >>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ >>>>>> >>>>> Looks good, especially eliminating the need for two different >>>>> append methods. I'll update based on this and other suggestions. >>>>> >>>>> /Claes >>>>>> Regards, Peter >>>>
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev. /Claes
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); } it appears the 'else" is missing? or I read it wrong? -Sherman On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic? /Claes On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
I don''t think an exponent should ever have a "dot', it always a signed integer. I think we can just remove the dead code, maybe put some wording to explain why no group, no dot here. -Sherman On 09/23/2014 12:42 PM, Claes Redestad wrote:
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic?
/Claes
On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
How about: // Specialized localization of exponents, where the source value can only // contain characters '0' through '9', starting at index offset, and no // group separators is added for any locale. private void localizedMagnitudeExp(StringBuilder sb, char[] value, final int offset, Locale l) { char zero = getZero(l); int len = value.length; for (int j = offset; j < len; j++) { char c = value[j]; sb.append((char) ((c - '0') + zero)); } } Webrev including this and the fixes for Conversion.SCIENTIFIC: http://cr.openjdk.java.net/~redestad/8050142/webrev.09/ /Claes On 2014-09-23 22:12, Xueming Shen wrote:
I don''t think an exponent should ever have a "dot', it always a signed integer. I think we can just remove the dead code, maybe put some wording to explain why no group, no dot here.
-Sherman
On 09/23/2014 12:42 PM, Claes Redestad wrote:
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic?
/Claes
On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote:
Hi,
Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, which would be unacceptable. This version takes the minimal approach to addressing this by adding back a method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting):
http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h...
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
Looks good. On 09/23/2014 02:07 PM, Claes Redestad wrote:
How about:
// Specialized localization of exponents, where the source value can only // contain characters '0' through '9', starting at index offset, and no // group separators is added for any locale. private void localizedMagnitudeExp(StringBuilder sb, char[] value, final int offset, Locale l) { char zero = getZero(l);
int len = value.length; for (int j = offset; j < len; j++) { char c = value[j]; sb.append((char) ((c - '0') + zero)); } }
Webrev including this and the fixes for Conversion.SCIENTIFIC: http://cr.openjdk.java.net/~redestad/8050142/webrev.09/
/Claes
On 2014-09-23 22:12, Xueming Shen wrote:
I don''t think an exponent should ever have a "dot', it always a signed integer. I think we can just remove the dead code, maybe put some wording to explain why no group, no dot here.
-Sherman
On 09/23/2014 12:42 PM, Claes Redestad wrote:
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic?
/Claes
On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote:
On 09/22/2014 12:43 PM, Claes Redestad wrote: > Hi, > > Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, > which would be unacceptable. This version takes the minimal approach to addressing this by adding back a > method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting): > > http://cr.openjdk.java.net/~redestad/8050142/webrev.07/ > > This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see > http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h... >
Hi Claes,
Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
Shouldn't this one be at the same class (FormatSpecifier?) as the "localizedMagnitudeExp"? the webrev shows it is one level up. Btw, when we are here, maybe we can just remove the FormatSpecifier.getZero(), it appears we have a static version already at Formatter level. On 09/24/2014 10:41 AM, Xueming Shen wrote:
Looks good.
On 09/23/2014 02:07 PM, Claes Redestad wrote:
How about:
// Specialized localization of exponents, where the source value can only // contain characters '0' through '9', starting at index offset, and no // group separators is added for any locale. private void localizedMagnitudeExp(StringBuilder sb, char[] value, final int offset, Locale l) { char zero = getZero(l);
int len = value.length; for (int j = offset; j < len; j++) { char c = value[j]; sb.append((char) ((c - '0') + zero)); } }
Webrev including this and the fixes for Conversion.SCIENTIFIC: http://cr.openjdk.java.net/~redestad/8050142/webrev.09/
/Claes
On 2014-09-23 22:12, Xueming Shen wrote:
I don''t think an exponent should ever have a "dot', it always a signed integer. I think we can just remove the dead code, maybe put some wording to explain why no group, no dot here.
-Sherman
On 09/23/2014 12:42 PM, Claes Redestad wrote:
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic?
/Claes
On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote:
On 2014-09-23 21:14, Xueming Shen wrote: > On 09/22/2014 12:43 PM, Claes Redestad wrote: >> Hi, >> >> Sherman pointed out that there was a path that could actually take a minor performance hit from this patch, >> which would be unacceptable. This version takes the minimal approach to addressing this by adding back a >> method operating on a char[], simplified for the specific usage case (the exponent part of a %g double formatting): >> >> http://cr.openjdk.java.net/~redestad/8050142/webrev.07/ >> >> This latest patch passes using the extended test coverage of java.util.Formatter I've proposed in 8058887, see >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h... >> > > Hi Claes, > > Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case? > It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping > the simple char[] for exp.
You're right, of course. I'll update the webrev.
/Claes
On 2014-09-24 20:14, Xueming Shen wrote:
Shouldn't this one be at the same class (FormatSpecifier?) as the "localizedMagnitudeExp"? the webrev shows it is one level up.
Nice catch!
Btw, when we are here, maybe we can just remove the FormatSpecifier.getZero(), it appears we have a static version already at Formatter level.
They are not identical, and in fact, I think some subtle things would change if localizedMagnitudeExp isn't moved up: FormatSpecifier.getZero() check versus the locale the Formatter was created with while the static method compares against Locale.US. This changes semantics when calling format operations with null, which is a legal option: Formatter f = new Formatter(Locale.X); // where Locale.X is a locale where zero != '0' f.format(null, "%.0e", 1000000000.0); // would print the exponent in different ways However, current behavior might actually be wrong since it's in contradiction to the specification, e.g., Formatter#format(Locale l, String format, Object ... args): * @param l * The {@linkplain java.util.Locale locale} to apply during * formatting. If {@code l} is {@code null} then no localization * is applied. This does not change this object's locale that was * set during construction. Most code interprets l == null like this, but FormatSpecifier.getZero() is an exception. I think this needs to be investigated and merits a bug of its own. Updated localizedMagnitudeExp to reside in FormatSpecifier, leaving getZero alone: http://cr.openjdk.java.net/~redestad/8050142/webrev.10/ /Claes
On 09/24/2014 10:41 AM, Xueming Shen wrote:
Looks good.
On 09/23/2014 02:07 PM, Claes Redestad wrote:
How about:
// Specialized localization of exponents, where the source value can only // contain characters '0' through '9', starting at index offset, and no // group separators is added for any locale. private void localizedMagnitudeExp(StringBuilder sb, char[] value, final int offset, Locale l) { char zero = getZero(l);
int len = value.length; for (int j = offset; j < len; j++) { char c = value[j]; sb.append((char) ((c - '0') + zero)); } }
Webrev including this and the fixes for Conversion.SCIENTIFIC: http://cr.openjdk.java.net/~redestad/8050142/webrev.09/
/Claes
On 2014-09-23 22:12, Xueming Shen wrote:
I don''t think an exponent should ever have a "dot', it always a signed integer. I think we can just remove the dead code, maybe put some wording to explain why no group, no dot here.
-Sherman
On 09/23/2014 12:42 PM, Claes Redestad wrote:
Ouch... but wait... the char[] returned from sun.misc.FormattedFloatingDecimal.getExponent() can never contain a '.', so we'll never find a dot here. Remove the dead code or fix the logic?
/Claes
On 2014-09-23 21:30, Xueming Shen wrote:
Also the logic in the loop of localizedMagnitudeExp() does not look correct. Shouldn't it be
char c= value[j]; if (c == '.') { append l10n dot... } else { sb.append(c - '0' + zero); }
it appears the 'else" is missing? or I read it wrong?
-Sherman
On 09/23/2014 12:27 PM, Claes Redestad wrote: > On 2014-09-23 21:14, Xueming Shen wrote: >> On 09/22/2014 12:43 PM, Claes Redestad wrote: >>> Hi, >>> >>> Sherman pointed out that there was a path that could actually >>> take a minor performance hit from this patch, >>> which would be unacceptable. This version takes the minimal >>> approach to addressing this by adding back a >>> method operating on a char[], simplified for the specific >>> usage case (the exponent part of a %g double formatting): >>> >>> http://cr.openjdk.java.net/~redestad/8050142/webrev.07/ >>> >>> This latest patch passes using the extended test coverage of >>> java.util.Formatter I've proposed in 8058887, see >>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.h... >>> >>> >> >> Hi Claes, >> >> Shouldn't we also keep the exp as char[] as well in "c == >> Conversion.SCIENTIFIC" case? >> It appears the usage of exp is the same as the one in >> "GENERAL", in which we are keeping >> the simple char[] for exp. > > You're right, of course. I'll update the webrev. > > /Claes
On 09/24/2014 01:33 PM, Claes Redestad wrote:
Updated localizedMagnitudeExp to reside in FormatSpecifier, leaving getZero alone:
I think we are good to go :-) I was wrong on getZeron.
I can push this change, Claes. -Brent
Sherman, Brent, thanks for reviewing and sponsoring this! /Claes On 2014-09-24 23:16, Brent Christian wrote:
I can push this change, Claes.
-Brent
I had to make an update to satisfy jcheck, and neglected to specify the user when re-committing, so I'm listed as the author. Sorry! -Brent On 9/24/14 2:16 PM, Brent Christian wrote:
I can push this change, Claes.
-Brent
Hi Claes, it looks good. And I have one minor improvement. Take look at the following lines of the method FormatSpecifier#hexDouble(double, int): 3532 // Get exponent and append at the end. 3533 String exp = res.substring(idx + 1); 3534 int iexp = Integer.parseInt(exp) -54; 3535 return res.substring(0, idx) + "p" 3536 + Integer.toString(iexp); The lines 3535-3536 are translated by the compiler to: new StringBuilder().append(res.substring(0, idx)).append("p").append(Integer.toString(iexp)).toString(); If we rewrite it to use StringBuilder, we can avoid creation of intermediate String objects, e.g.: 3535 return new StringBuilder().append(res, 0, idx).append('p') 3536 .append(iexp).toString(); But I'm not sure how common is the usage of #hexDouble-method. Maybe this change is not worth it. Best regards, Andrej Golovnin On Mon, Jul 14, 2014 at 2:23 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
Hi Andrej, while the change seems reasonable, I can't find any usage of %a/%A in the JDK and haven't even covered it in my micros. :-) Also, it could be further improved with 8041972 to turn: 3533 String exp = res.substring(idx + 1); 3534 int iexp = Integer.parseInt(exp) -54; 3533 int exp = Integer.parseInt(res, 10, idx + 1); I've planned to file a follow-on RFE to incorporate improvements made possible by 8041972 (like the one above); maybe we should roll this suggestion into that follow-up and add some micros to measure that we're actually improving? /Claes On 07/14/2014 03:09 PM, Andrej Golovnin wrote:
Hi Claes,
it looks good. And I have one minor improvement. Take look at the following lines of the method FormatSpecifier#hexDouble(double, int):
3532 // Get exponent and append at the end. 3533 String exp = res.substring(idx + 1); 3534 int iexp = Integer.parseInt(exp) -54; 3535 return res.substring(0, idx) + "p" 3536 + Integer.toString(iexp);
The lines 3535-3536 are translated by the compiler to:
new StringBuilder().append(res.substring(0, idx)).append("p").append(Integer.toString(iexp)).toString();
If we rewrite it to use StringBuilder, we can avoid creation of intermediate String objects, e.g.:
3535 return new StringBuilder().append(res, 0, idx).append('p') 3536 .append(iexp).toString();
But I'm not sure how common is the usage of #hexDouble-method. Maybe this change is not worth it.
Best regards, Andrej Golovnin
On Mon, Jul 14, 2014 at 2:23 PM, Claes Redestad <claes.redestad@oracle.com <mailto:claes.redestad@oracle.com>> wrote:
Hi again,
updated webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.1 <http://cr.openjdk.java.net/%7Eredestad/8050142/webrev.1>
changes: - specify capacity on line 2931 as suggested by Andrej Golovnin - exp.append("0") -> exp.append('0') on line 3781 - merged append+justify into appendJustified as suggested by Peter Levart - replaced the reoccuring pattern of appending a number of zeros into a call to trailingZeros
performance difference seemingly at noise levels in micros, but bonus to readability and Formatter*.class-files are now a total of 246 bytes smaller
/Claes
On 2014-07-14 13:29, Claes Redestad wrote:
Hi Peter,
On 2014-07-14 13:25, Peter Levart wrote:
On 07/14/2014 12:07 PM, Claes Redestad wrote:
Hi,
please review this patch which optimizes away some allocations from java.util.Formatter and achieve 1.1-1.3x speedups of micros targetting String.format. See bug for more details.
webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0 <http://cr.openjdk.java.net/%7Eredestad/8050142/webrev.0> bug: https://bugs.openjdk.java.net/browse/JDK-8050142
Testing: JPRT, jtreg (java/lang/String, java/util/Formatter), SPECjbb2013 and microbenchmarks
Thanks!
/Claes
Hi Claes,
Since justify() result is always appended to the resulting Appendable, you could merge the functionality and eliminate constructing intermediary StringBuilder altogether:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/Formatter/webrev.01/>
Looks good, especially eliminating the need for two different append methods. I'll update based on this and other suggestions.
/Claes
Regards, Peter
participants (8)
-
Andrej Golovnin
-
Brent Christian
-
Claes Redestad
-
Ivan Gerasimov
-
Jason Mehrens
-
Marcus Lagergren
-
Peter Levart
-
Xueming Shen