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

[Claes Redestad]

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