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

Tue Sep 23 19:14:11 UTC 2014

On 09/22/2014 12:43 PM, Claes Redestad wrote:
> 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
>

Hi Claes,

Shouldn't we also keep the exp as char[] as well in "c == Conversion.SCIENTIFIC" case?
It appears the usage of exp is the same as the one in "GENERAL", in which we are keeping
the simple char[] for exp.

Thanks!
-Sherman

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