[PATCH] regex matcher opt: remove redundant StringBuilder

[Isaac Levy]

Hi Sherman,

Thanks for clarifying. Looks like exceptions are caused by invalid
format string. I wouldn't expect most programs to be catching this and
preserving their buffer, but dunno.

How much does it affect perf? Well it depends on use case, a jmh of
replaceAll with a length 200 string of digits and regex "(\w)" shows
about 30% speedup.

[info] Benchmark          Mode  Cnt   Score   Error  Units
[info] origM  avgt   10  11.669 ± 0.211  us/op
[info] newM   avgt   10   8.926 ± 0.095  us/op

Isaac


On Tue, Apr 24, 2018 at 12:53 PM, Xueming Shen <xueming.shen at oracle.com> wrote:
> Hi Isaac,
>
> I actually meant to say "we are not supposed to output the partial text into
> the output buffer in case of an exception". It has nothing to do with the
> changeset you cited. This has been the behavior since day one/JDK1.4,
> though it is not specified explicitly in the API doc. The newly added
> StringBuilder variant simply follows this behavior. If it's really desired
> it
> is kinda doable to save that StringBuilder without the "incompatible"
> behavior
> change but just wonder if it is really worth the effort.
>
> Thanks,
> Sherman
>
>
> On 4/24/18, 9:11 AM, Isaac Levy wrote:
>>
>> (moving this to a separate discussion)
>>
>>
>> --- a/src/java.base/share/classes/java/util/regex/Matcher.java
>> +++ b/src/java.base/share/classes/java/util/regex/Matcher.java
>> @@ -993,13 +993,11 @@
>>      public Matcher appendReplacement(StringBuilder sb, String
>> replacement) {
>>           // If no match, return error
>>           if (first<  0)
>>               throw new IllegalStateException("No match available");
>> -        StringBuilder result = new StringBuilder();
>> -        appendExpandedReplacement(replacement, result);
>>           // Append the intervening text
>>           sb.append(text, lastAppendPosition, first);
>>           // Append the match substitution
>> +        appendExpandedReplacement(replacement, sb);
>> -        sb.append(result);
>>
>>
>>
>> On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen<xueming.shen at oracle.com>
>> wrote:
>>>
>>>
>>> I would assume in case of an exception thrown from
>>> appendExpandedReplacement() we don't
>>> want "text" to be pushed into the "sb".
>>>
>>> -sherman
>>
>>
>> Perhaps. Though the behavior under exception is undefined and this
>> function is probably primarily used though the replaceAll API, which
>> wouldn’t return the intermediate sb under the case of exception.
>>
>> My reading of the blame was the temp StringBuilder was an artifact of
>> the api previously using StringBuffer externally.  See
>> http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3
>
>