RFR 8222806: Inefficient String.replace in PathFileObject.toBinaryName

[Ivan Gerasimov]

Hi Ron!

If it is known that sep.length() is always == 1, then maybe just add an 
assert and always use replace(char, char) variant?

Also, it seems that you can safely replace fileName.lastIndexOf(".") and 
s.lastIndexOf("/") with fileName.lastIndexOf('.') and s.lastIndexOf('/').

With kind regards,

Ivan


On 4/23/19 12:32 PM, Ron Shapiro 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
>>

-- 
With kind regards,
Ivan Gerasimov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190423/5a0b7f10/attachment.html>