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