Optimize Formatter.parse (including String.printf)
|
View:
New views
7 Messages
—
Rating Filter:
Alert me
|
 |
Optimize Formatter.parse (including String.printf)
by martinrb
::
Rate this Message:
Hi Sherman,
I'd like you to do a code review. http://cr.openjdk.java.net/~martin/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 |
|
|
Re: Optimize Formatter.parse (including String.printf)
by xueming.shen
::
Rate this Message:
I will take a look.
The bugid# is 6898220. 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 |
|
|
Re: Optimize Formatter.parse (including String.printf)
by xueming.shen
::
Rate this Message:
#2659:
conversion(m.group(idx++)); -> conversion(m.group(idx)); ? 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 |
|
|
Re: Optimize Formatter.parse (including String.printf)
by martinrb
::
Rate this Message:
On Wed, Nov 4, 2009 at 23:07, Xueming Shen <Xueming.Shen@...> 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 > > |
|
|
Re: Optimize Formatter.parse (including String.printf)
by David Schlosnagle
::
Rate this Message:
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@...> wrote: > On Wed, Nov 4, 2009 at 23:07, Xueming Shen <Xueming.Shen@...> 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 >> >> > |
|
|
Re: Optimize Formatter.parse (including String.printf)
by martinrb
::
Rate this Message:
On Thu, Nov 5, 2009 at 15:22, David Schlosnagle <schlosna@...> 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@...> wrote: >> On Wed, Nov 4, 2009 at 23:07, Xueming Shen <Xueming.Shen@...> 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 >>> >>> >> > |
|
|
Re: Optimize Formatter.parse (including String.printf)
by xueming.shen
::
Rate this Message:
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 <martinrb@...> wrote: > >> On Wed, Nov 4, 2009 at 23:07, Xueming Shen <Xueming.Shen@...> 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 >>>> >>> |
| Free embeddable forum powered by Nabble | Forum Help |
