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

[Marcus Lagergren]

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