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

[Claes Redestad]

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

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