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.christ...@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.redes...@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 >>>>>>> >>>>>>> >>>> >>
