RFR 8222806: Inefficient String.replace in PathFileObject.toBinaryName

[Martin Buchholz]

I'm opposed to adding length-1 special cases to String.replace.

OTOH It's reasonable to add a bit of low-level code to javac's toBinaryName
to save 1% cpu.
I might replace the separator and remove the extension in a single pass,
perhaps using StringBuilder.
All known filesystems have a separator of length 1, I think.
The input filename can have only one "." char, I think.

On Tue, Apr 23, 2019 at 12:33 PM Ron Shapiro <ronshapiro at google.com> wrote:

> 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/c02c1448/attachment.html>