RFR 8222806: Inefficient String.replace in PathFileObject.toBinaryName

[Ron Shapiro]

I tried this, but it was 3.5x slower (320ms vs. 95ms) than the
removeExtension().replace(char, char) version. I imagine that's due to
something in the JIT for replace() vs. my personal StringBuilder usage.

Are we able to assume that the seperator length is always 1? It doesn't
seem so from the FileSystem contract, but we could assert that that's all
JavacFileManager supports. I'm not sure I see much value in doing so vs.
supporting both options since it's trivial to do so.

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

> I might replace the separator and remove the extension in a single pass,
> perhaps using StringBuilder.
>
> That may be worth investigating.
>
> -- Jon
>
> On 04/23/2019 01:26 PM, Martin Buchholz wrote:
>
> 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/20190424/311ce2ec/attachment.html>