RFR [9]: 8050142: Optimize java.util.Formatter

[Brent Christian]

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 at oracle.com
>>> To: core-libs-dev at 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
>>>>>>
>>>>>>
>>>
>