Re: Optimize Formatter.parse (including String.printf)

[Xueming Shen]

Looks fine.

Agreed the line commented out should be  removed...but yes, we have 
other leftover:-)

David Schlosnagle wrote:
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 <marti...@google.com> wrote:
  
On Wed, Nov 4, 2009 at 23:07, Xueming Shen <xueming.s...@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