On 4/3/14 4:43 PM, Jeremy Manson wrote:
Good catch, thanks.
I think we should probably just go with the (equivalent to the)
StringBuffer variant. I'm pretty loathe to modify the StringBuilder
directly if we are going to back that change out.
Do you want me to generate a new patch?
I can/will send out an updated webrev before push.
-Sherman
Jeremy
On Thu, Apr 3, 2014 at 2:27 PM, Xueming Shen <xueming.s...@oracle.com
<mailto:xueming.s...@oracle.com>> wrote:
On 03/25/2014 02:07 PM, Jeremy Manson wrote:
Okay. Thanks, Sherman. Here's an updated version.
I've diverged a bit from Peter's version. In this version,
appendExpandedReplacement takes a StringBuilder. The
implications is that In the StringBuilder case, it saves creating
a new StringBuilder object. In the StringBuffer case, it creates
a new StringBuilder, but it doesn't have to acquire and release
all of those locks.
Hi Jeremy,
It appears the "optimized" StringBuilder version will cause
following test case failure,
in which the "xyz" will be copied into the result buffer, even
when the replacement
string triggers a IAE.
// Check nothing has been appended into the output buffer if
// the replacement string triggers IllegalArgumentException.
Pattern p = Pattern.compile("(abc)");
Matcher m = p.matcher("abcd");
StringBuilder result = new StringBuilder();
try {
m.appendReplacement(result, ("xyz$g"));
} catch (IllegalArgumentException iae) {
if (result.length() != 0)
System.err.println(" FAILED");
}
We may have to either catch the IAE and reset the sb, or create
a new sb, as the StringBuffer does.
-Sherman
I also noticed a redundant cast to (int), which I removed.
Jeremy
On Fri, Mar 21, 2014 at 7:34 PM, Xueming Shen
<xueming.s...@oracle.com <mailto:xueming.s...@oracle.com>> wrote:
let's add the StringBuilder method(s), if you can provide an
updated version, I can run the rest (since it's
to add new api, there is an internal CCC process need to go
through).
-Sherman
On 3/21/14 5:18 PM, Jeremy Manson wrote:
So, this is all a little opaque to me. How do we make the
go/no-go decision on something like this? Everyone who has
chimed in seems to think it is a good idea.
Jeremy
On Thu, Mar 20, 2014 at 10:38 AM, Jeremy Manson
<jeremyman...@google.com <mailto:jeremyman...@google.com>>
wrote:
Sherman,
If you had released it then (which you wouldn't have
been able to do, because you would have to wait another
two years for Java 7), you would have found that it
improved performance even with C2. It is only
post-escape-analysis that the performance in C2 equalized.
Anyway, I think adding the StringBuilder variant and
deferring / dealing with the Appendable differently is
the right approach, FWIW.
Jeremy
On Thu, Mar 20, 2014 at 10:25 AM, Xueming Shen
<xueming.s...@oracle.com
<mailto:xueming.s...@oracle.com>> wrote:
2009? I do have something similar back to 2009 :-)
http://cr.openjdk.java.net/~sherman/regex_replace/webrev/
<http://cr.openjdk.java.net/%7Esherman/regex_replace/webrev/>
Then the ball was dropped around the discussion of
whether or not
the IOE should be thrown.
But if we are going to/have to have explicit
StringBuilder/Buffer pair
anyway, then we can keep the Appendable version as
private for now
and deal with the StringBuilder and Appendable as
two separate
issues.
-Sherman
On 03/20/2014 09:52 AM, Jeremy Manson wrote:
That's definitely an improvement - I think that
when I wrote this (circa
2009), I didn't think about Appendable.
I take it my argument convinced someone? :)
Jeremy
On Thu, Mar 20, 2014 at 1:32 AM, Peter
Levart<peter.lev...@gmail.com
<mailto:peter.lev...@gmail.com>>wrote:
On 03/19/2014 06:51 PM, Jeremy Manson wrote:
I'm told that the diff didn't make it.
I've put it in a Google drive
folder...
https://drive.google.com/file/d/0B_GaXa6O4K5LY3Y0aHpranM3aEU/
edit?usp=sharing
Jeremy
Hi Jeremy,
Your factoring-out of expandReplacement()
method exposed an opportunity to
further optimize the code. Instead of
creating intermediate StringBuilder
instance for each expandReplacement() call,
this method could append
directly to resulting
StringBuffer/StringBuilder, like in the
following:
http://cr.openjdk.java.net/~plevart/jdk9-dev/MatcherWithStringBuilder/
<http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/MatcherWithStringBuilder/>
webrev.01/
Regards, Peter
On Wed, Mar 19, 2014 at 10:41 AM, Jeremy
Manson<jeremyman...@google.com
<mailto:jeremyman...@google.com>>
wrote:
Hi folks,
We've had this internally for a
while, and I keep meaning to bring it up
here. The Matcher class has a few
public methods that take
StringBuffers,
and we've found it useful to add
similar versions that take
StringBuilders.
It has two benefits:
- Users don't have to convert from
one to the other when they want to use
the method in question. The
symmetry is nice.
- The StringBuilder variants are
faster (if lock optimizations don't kick
in, which happens in the interpreter
and the client compiler). For
interpreted / client-compiled code,
we saw something like a 25% speedup
on
String.replaceAll(), which calls
into this code.
Any interest? Diff attached.
Jeremy
