Use StringBuilder in String.replace (2x faster)

[fvasco]

The current String.replace implementation use splitToSequence, it is pretty slow.

I made a test project to improve String.replace's performance, available here: https://github.com/fvasco/kotlin-string-replace/blob/master/src/main/kotlin/org/sample/MyBenchmark.kt

Results are:

Benchmark                     Mode  Cnt        Score       Error  Units
MyBenchmark.replaceNew       thrpt    5  1412205,791 ± 12644,734  ops/s
MyBenchmark.replaceOriginal  thrpt    5   697439,698 ±  5664,105  ops/s
MyBenchmark.replaceRegex     thrpt    5  1098579,545 ± 11091,188  ops/s

The regex version is a little faster, but it can be considered in order to reduce the bytecode.
The new version is the case-insenditive translation of the Java native version.
In both version the platform implementation has been preferred in order to delegate the maintenance.

Original discussion here: https://discuss.kotlinlang.org/t/string-replace-implementation-is-very-poor/19026

[ilya-g]

Thanks for taking time and thoroughly benchmarking this. Could you also open a corresponding issue in a tracker?

[ilya-g]

Could you also post benchmark results with ignoreCase = false?

[fvasco]

Your welcome,
see https://youtrack.jetbrains.com/issue/KT-41799

[fvasco]

Could you also post benchmark results with ignoreCase = false?

Benchmark                     Mode  Cnt         Score       Error  Units
MyBenchmark.replaceNew       thrpt    5  45009927,400 ± 50963,601  ops/s
MyBenchmark.replaceOriginal  thrpt    5   4118892,481 ± 21614,632  ops/s
MyBenchmark.replaceRegex     thrpt    5  44966941,400 ± 85611,686  ops/s

Edit: in case of no substitution

[fvasco]

Could you also post benchmark results with ignoreCase = false?

Benchmark                     Mode  Cnt        Score        Error  Units
MyBenchmark.replaceNew       thrpt    5  5812212,786 ±  64853,335  ops/s
MyBenchmark.replaceOriginal  thrpt    5  2636245,811 ± 188415,757  ops/s
MyBenchmark.replaceRegex     thrpt    5  5761148,430 ±  39618,165  ops/s

I pushed the relative commit to the repository

[derektom14]

I would add a comment explaining how this overflow proves that it's impossible to build the desired string, and maybe even include some of the length information in a message in the OutOfMemoryError.

[fvasco]

I would add a comment explaining how this overflow proves that it's impossible to build the desired string

Something like: "detect Int overflow"?

maybe even include some of the length information in a message in the OutOfMemoryError

Can you provide a suggestion?
Please consider that every append in this function can cause a similar error, this hint works wello only for a single replacement.

[derektom14]

I would expand on it a bit more: "even a single replacement would create a String with more than Int.MAX_VALUE characters, not allowed"
With your point about subsequent replacements causing the same error, I suppose that the errors should appear uniform, so OutOfMemoryError() is fine.

[ilya-g]

I've run the provided benchmark and got somewhat similar results. After that I decided to perform a more extensive benchmark: i.e to test each implementation individually (manual, regex-based, delegating to the platform, the current one in stdlib) varying the following parameters: JDK version (8 or 11), the length of an input string, the number of occurrences of a string being searched, case sensitivity, the amount of backtracking required.

The benchmarks and their results can be found here: https://github.com/ilya-g/string-replace-benchmark

Based on these findings I'd recommend taking the manual implementation for case-sensitive replacement and the regex-based (but with manual replacement, see replaceRegex1) for case-insensitive replacement.

[ilya-g]

So, if there are no objections, I'll take the proposed implementation by @fvasco and modify it according to the conclusions from the last benchmark.

Based on these findings I'd recommend taking the manual implementation for case-sensitive replacement and the regex-based (but with manual replacement, see replaceRegex1) for case-insensitive replacement.

[fvasco]

Regardless the benchmark results, I wish to add that the platform implementation reduces the binary size and delegates the responsibility.

[ilya-g]

I've merged it manually and applied the proposed changes on top of it: fafb4a7~2...fafb4a7

Thanks again for the PR.

[fvasco]

...and kudos to all discussion creator and contributors