You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "tbw777 (via GitHub)" <gi...@apache.org> on 2023/02/02 12:43:39 UTC
[GitHub] [tomcat] tbw777 opened a new pull request, #581: Speedup by removing non pattern replaceAll with constant arg
tbw777 opened a new pull request, #581:
URL: https://github.com/apache/tomcat/pull/581
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094835289
##########
java/org/apache/tomcat/buildutil/translate/Utils.java:
##########
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("\\\\$1");
- result = result.replaceAll("\t", "\\t");
+ result = result.replace("\t", "t");
Review Comment:
This is not correct. The literal (unescaped) replacement text should be `\t`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] happyhua commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "happyhua (via GitHub)" <gi...@apache.org>.
happyhua commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1454685803
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094893763
##########
java/org/apache/tomcat/buildutil/translate/Utils.java:
##########
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("\\\\$1");
- result = result.replaceAll("\t", "\\t");
+ result = result.replace("\t", "t");
Review Comment:
For the record, none of the current i18n strings use tabs. I did find some messages using them in early 7.0.x versions, but those messages were never referenced in the Java code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1095639347
##########
java/org/apache/tomcat/buildutil/translate/Utils.java:
##########
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("\\\\$1");
- result = result.replaceAll("\t", "\\t");
+ result = result.replace("\t", "t");
Review Comment:
Tx.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094884701
##########
java/org/apache/tomcat/buildutil/translate/Utils.java:
##########
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("\\\\$1");
- result = result.replaceAll("\t", "\\t");
+ result = result.replace("\t", "t");
Review Comment:
It is a bug in the original code. It should be replacing all tabs with the literal string `\t`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] tbw777 commented on a diff in pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "tbw777 (via GitHub)" <gi...@apache.org>.
tbw777 commented on code in PR #581:
URL: https://github.com/apache/tomcat/pull/581#discussion_r1094863229
##########
java/org/apache/tomcat/buildutil/translate/Utils.java:
##########
@@ -123,7 +123,7 @@ static String formatValueCommon(String in) {
result = ESCAPE_LEADING_SPACE.matcher(result).replaceAll("\\\\$1");
- result = result.replaceAll("\t", "\\t");
+ result = result.replace("\t", "t");
Review Comment:
The second argument in replaceAll means escape t, not \t.
You can check proofs tests. equals returning true if replace maked to t, not \t
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "ChristopherSchultz (via GitHub)" <gi...@apache.org>.
ChristopherSchultz commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416024123
Apologies: my comments are rubbish after Java 9. Please forgive the noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "ChristopherSchultz (via GitHub)" <gi...@apache.org>.
ChristopherSchultz commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416015385
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_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "ChristopherSchultz (via GitHub)" <gi...@apache.org>.
ChristopherSchultz commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1416017516
I think this "performance improvement" is not an improvement and doesn't affect performance in any meaningful way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf merged pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf merged PR #581:
URL: https://github.com/apache/tomcat/pull/581
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] tbw777 commented on pull request #581: Speedup by removing non pattern replaceAll with constant arg
Posted by "tbw777 (via GitHub)" <gi...@apache.org>.
tbw777 commented on PR #581:
URL: https://github.com/apache/tomcat/pull/581#issuecomment-1414189316
@markt-asf
Branch was updated and https://gist.github.com/tbw777/8a6ef60af21487c5faec67037099fd0b also
Check please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org