Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speedup by removing non pattern replaceAll with constant arg #581

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Speedup by removing non pattern replaceAll with constant arg #581

merged 1 commit into from
Feb 3, 2023

Conversation

tbw777
Copy link
Contributor

@tbw777 tbw777 commented Feb 2, 2023

replaceAll("ABC", "") is non Pattern method and therefore must be replaced to simple fast replace()
A proofs of changes: https://gist.github.com/tbw777/8a6ef60af21487c5faec67037099fd0b

Copy link
Contributor

@markt-asf markt-asf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR - these are good changes to have.

replaceAll("ABC", "") is non Pattern method and therefore must be replaced to simple fast replace()
A proofs of changes: https://gist.github.com/tbw777/8a6ef60af21487c5faec67037099fd0b
@tbw777
Copy link
Contributor Author

tbw777 commented Feb 2, 2023

@markt-asf
Branch was updated and https://gist.github.com/tbw777/8a6ef60af21487c5faec67037099fd0b also
Check please.

@markt-asf markt-asf merged commit 399ea1b into apache:main Feb 3, 2023
@ChristopherSchultz
Copy link
Contributor

ChristopherSchultz commented Feb 3, 2023

There isn't much of a difference between String.replace and String.replaceAll. They both use regular expressions under the hood. Java's Pattern class has a special case for "this search-string doesn't include any metacharacters" (it's a flag called LITERAL) and has optimized code-paths for those cases.

IMO replaceAll is more clear than replace because it's absolutely clear that all instances will be replaced in the string.

If performance is the primary factor, here, we should use an implementation of string-replacement that does not involve creating Pattern and Matcher objects and re-encoding the replacement String every single time.

@ChristopherSchultz
Copy link
Contributor

ChristopherSchultz commented Feb 3, 2023

I think this "performance improvement" is not an improvement and doesn't affect performance in any meaningful way.

@ChristopherSchultz
Copy link
Contributor

Apologies: my comments are rubbish after Java 9. Please forgive the noise.

Stephan202 referenced this pull request in google/error-prone Feb 27, 2023
RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=180751675
@happyhua
Copy link

happyhua commented Mar 4, 2023

A replaceAll -> replace change in for example java/org/apache/tomcat/buildutil/translate/Utils.java can produce different output.

        String source = "1\t2\t3";

        System.out.println("source: " + source);
        System.out.println("replaceAll: " + source.replaceAll("\t", "\\t"));
        System.out.println("replace: " + source.replace("\t", "\\t"));

source: 1	2	3
replaceAll: 1t2t3
replace: 1\t2\t3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants