RFR 8222806: Inefficient String.replace in PathFileObject.toBinaryName

[Jonathan Gibbons]

> 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 
> <mailto: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 <mailto: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 <mailto: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
>>             <mailto: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/
>>>                 <http://cr.openjdk.java.net/%7Eronsh/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/414dd2f2/attachment-0001.html>