Optimize Formatter.parse (including String.printf)

[Martin Buchholz]

On Thu, Nov 5, 2009 at 15:22, David Schlosnagle <schlosna at gmail.com> wrote:
> Martin,
>
> Did you want to remove the commented out formatter?
>
>  //private Formatter formatter;

Yes, thanks, a mercurial glitch.
Webrev already updated.

http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Formatter.parse/

Martin

>
> - Dave Schlosnagle
>
>
> On Thu, Nov 5, 2009 at 6:14 PM, Martin Buchholz <martinrb at google.com> wrote:
>> On Wed, Nov 4, 2009 at 23:07, Xueming Shen <Xueming.Shen at sun.com> wrote:
>>> #2659:
>>>
>>> conversion(m.group(idx++));
>>>
>>> ->  conversion(m.group(idx)); ?
>>
>> Done.
>>
>> I couldn't help making this additonal change:
>>
>> diff --git a/src/share/classes/java/util/Formatter.java
>> b/src/share/classes/java/util/Formatter.java
>> --- a/src/share/classes/java/util/Formatter.java
>> +++ b/src/share/classes/java/util/Formatter.java
>> @@ -2503,7 +2503,7 @@
>>                     al.add(new FixedString(s.substring(i, m.start())));
>>                 }
>>
>> -                al.add(new FormatSpecifier(this, m));
>> +                al.add(new FormatSpecifier(m));
>>                 i = m.end();
>>             } else {
>>                 // No more valid format specifiers.  Check for possible invalid
>> @@ -2552,7 +2552,7 @@
>>         private boolean dt = false;
>>         private char c;
>>
>> -        private Formatter formatter;
>> +        //private Formatter formatter;
>>
>>         // cache the line separator
>>         private String ls;
>> @@ -2640,8 +2640,7 @@
>>             return c;
>>         }
>>
>> -        FormatSpecifier(Formatter formatter, Matcher m) {
>> -            this.formatter = formatter;
>> +        FormatSpecifier(Matcher m) {
>>             int idx = 1;
>>
>>             index(m.group(idx++));
>> @@ -2656,7 +2655,7 @@
>>                     f.add(Flags.UPPERCASE);
>>             }
>>
>> -            conversion(m.group(idx++));
>> +            conversion(m.group(idx));
>>
>>             if (dt)
>>                 checkDateTime();
>> @@ -2811,9 +2810,9 @@
>>
>>         private void printString(Object arg, Locale l) throws IOException {
>>             if (arg instanceof Formattable) {
>> -                Formatter fmt = formatter;
>> -                if (formatter.locale() != l)
>> -                    fmt = new Formatter(formatter.out(), l);
>> +                Formatter fmt = Formatter.this;
>> +                if (fmt.locale() != l)
>> +                    fmt = new Formatter(fmt.out(), l);
>>                 ((Formattable)arg).formatTo(fmt, f.valueOf(), width,
>> precision);
>>             } else {
>>                 if (f.contains(Flags.ALTERNATE))
>>
>> FormatConversion is an inner class, so already has
>> a pointer to its enclosing instance - no need to provide
>> an additional explicit one.
>>
>> Martin
>>
>>>
>>>
>>> The rest looks fine.
>>>
>>> sherman
>>>
>>>
>>> Martin Buchholz wrote:
>>>>
>>>> Hi Sherman,
>>>>
>>>> I'd like you to do a code review.
>>>>
>>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Formatter.parse/
>>>> <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/Formatter.parse/>
>>>>
>>>> Synopsis: Optimize Formatter.parse (including String.printf)
>>>> Description:
>>>> Formatter is not as efficient as it could be.
>>>> Here's a low-hanging fruit optimization
>>>> that creates fewer objects,
>>>> and uses the idiom
>>>> return al.toArray(new FormatString[al.size()]);
>>>>  I'm sure additional optimizations are possible.
>>>>
>>>> Results: about 10-20% faster on in-house microbenchmarks of
>>>> String.printf
>>>>
>>>> Martin
>>>
>>>
>>
>