Optimize Formatter.parse (including String.printf)

[David Schlosnagle]

Martin,

Did you want to remove the commented out formatter?

 //private Formatter formatter;

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