RFR 8222806: Inefficient String.replace in PathFileObject.toBinaryName

[Ron Shapiro]

Adding two of the latest authors to String.replace(CharSequence,
CharSequence) to see if it makes sense to special case when both sequences
have length() == 1 to redirect to String.replace(char, char).

On Tue, Apr 23, 2019 at 12:47 PM Jonathan Gibbons <
jonathan.gibbons at oracle.com> wrote:

> I think we should follow up with folk who understand performance
> trade-offs.
>
> Looking at String.replace, it already computes the length of the
> replacement string, so the default operational cost is just a single int
> comparison, but I agree the code would be more complex there, and that
> would be for the relevant experts to consider.
>
> -- Jon
> On 4/23/19 9:14 AM, Liam Miller-Cushon wrote:
>
> This does seem like a small trade-off between
> readability/conciseness/maintainability and performance. Jon, do you have
> suggestions for how to evaluate that trade-off?
>
> re: doing this in String.replace, I suspect that the usage in
> PathFileObject is unusually biased towards single-character arguments,
> since we'd only see longer arguments with a non-default filesystem with an
> unusual path separator. In the general case, String.replace is probably
> more likely to see longer arguments, and adding the additional logic and
> branches there may be less of a slam-dunk.
>
> On Mon, Apr 22, 2019 at 11:08 AM Ron Shapiro <ronshapiro at google.com>
> wrote:
>
>> I mentioned in the bug that it had a 300ms improvement for a 30s build.
>>
>> I think it would be reasonable in String.replace() if it was common
>> enough to be worth checking the lengths. I don't know what the benchmarking
>> considerations are there.
>>
>> On Mon, Apr 22, 2019 at 2:03 PM Jonathan Gibbons <
>> jonathan.gibbons at oracle.com> wrote:
>>
>>> That seems like micro-optimization.  Do you have numbers to back it up?
>>>
>>> Should this sort of optimization be done in String.replace?
>>>
>>> -- Jon
>>> On 4/22/19 8:51 AM, Ron Shapiro wrote:
>>>
>>> Hi,
>>>
>>> Please review this small change to improve the performance of
>>> PathFileObject.toBinaryName:
>>>
>>> webrev: http://cr.openjdk.java.net/~ronsh/8222806/webrev.00/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8222806
>>>
>>> Thanks!
>>> Ron
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190423/6834fc92/attachment-0001.html>