You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/02/29 14:14:04 UTC
[GitHub] [commons-lang] nstdio opened a new pull request #496: Avoid
unnecessary allocation in wrapIfMissing.
nstdio opened a new pull request #496: Avoid unnecessary allocation in wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496
Now `StringUtils#wrapIfMissing(String, char)` and `StringUtils#wrapIfMissing(String, String)` does not allocate buffer when input is already wrapped.
Here are the benchmark results on my machine:
```
Benchmark (input) Mode Samples Score Score error Units
optimized /should not be wrapped/ thrpt 20 357.849 4.525 ops/us
optimized should be wrapped thrpt 20 33.779 0.246 ops/us
original /should not be wrapped/ thrpt 20 35.581 0.236 ops/us
original should be wrapped thrpt 20 33.671 0.360 ops/us
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] nstdio commented on a change in pull request #496:
LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
nstdio commented on a change in pull request #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#discussion_r387595656
##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -9508,20 +9514,27 @@ public static String wrapIfMissing(final String str, final char wrapWith) {
* @param str
* the string to be wrapped, may be {@code null}
* @param wrapWith
- * the char that will wrap {@code str}
+ * the string that will wrap {@code str}
* @return the wrapped string, or {@code null} if {@code str==null}
* @since 3.5
*/
public static String wrapIfMissing(final String str, final String wrapWith) {
if (isEmpty(str) || isEmpty(wrapWith)) {
return str;
}
+
+ final boolean wrapStart = !str.startsWith(wrapWith);
+ final boolean wrapEnd = !str.endsWith(wrapWith);
+ if (!wrapStart && !wrapEnd) {
+ return str;
+ }
+
final StringBuilder builder = new StringBuilder(str.length() + wrapWith.length() + wrapWith.length());
Review comment:
@garydgregory Sure, that makes sense. Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] RameshDhungel commented on issue #496: LANG-1523:
Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
RameshDhungel commented on issue #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-596218820
Okay thank you, I did pull an new PR with the changes I described earlier. If you could let me know if I made any mistake I would appreciate it thank you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] coveralls edited a comment on issue #496: LANG-1523:
Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-592952175
[](https://coveralls.io/builds/29109735)
Coverage increased (+0.003%) to 95.104% when pulling **292c19404a4950b67599ea70041762371f2af099 on nstdio:optimize-wrap-if-missing** into **94b3784fdec5d0e9d63e4aec6772144b68283790 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] nstdio commented on issue #496: LANG-1523: Avoid
unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
nstdio commented on issue #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-593348020
Done.
@kinow
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory commented on issue #496: LANG-1523:
Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-596218362
@RameshDhungel I merged this PR four days ago. If you have something else in mind, please offer it up in a new PR. Thank you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory merged pull request #496: LANG-1523:
Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory commented on a change in pull request
#496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#discussion_r386596611
##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -9508,20 +9514,27 @@ public static String wrapIfMissing(final String str, final char wrapWith) {
* @param str
* the string to be wrapped, may be {@code null}
* @param wrapWith
- * the char that will wrap {@code str}
+ * the string that will wrap {@code str}
* @return the wrapped string, or {@code null} if {@code str==null}
* @since 3.5
*/
public static String wrapIfMissing(final String str, final String wrapWith) {
if (isEmpty(str) || isEmpty(wrapWith)) {
return str;
}
+
+ final boolean wrapStart = !str.startsWith(wrapWith);
+ final boolean wrapEnd = !str.endsWith(wrapWith);
+ if (!wrapStart && !wrapEnd) {
+ return str;
+ }
+
final StringBuilder builder = new StringBuilder(str.length() + wrapWith.length() + wrapWith.length());
Review comment:
Since the behavior has changed, I would expect a clarification in the Javadoc. Plus a matching test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] RameshDhungel commented on issue #496: LANG-1523:
Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
RameshDhungel commented on issue #496: LANG-1523: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-596217584
Hello, I just saw this issue in Jira, and if I understand the issue correctly. The StringUtils.wrapIfMissing is making a StringBuilder even when a string is already wrapped for example Str "xabx" with wrapper "x". Rather than not creating a instance of String Builder. What we can do is check if the string is already wrapped, if so then just return the String. This would avoid the String Builder being created.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] kinow commented on a change in pull request #496:
(doc): Avoid unnecessary allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #496: (doc): Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#discussion_r386083762
##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -9508,20 +9514,27 @@ public static String wrapIfMissing(final String str, final char wrapWith) {
* @param str
* the string to be wrapped, may be {@code null}
* @param wrapWith
- * the char that will wrap {@code str}
+ * the string that will wrap {@code str}
* @return the wrapped string, or {@code null} if {@code str==null}
* @since 3.5
*/
public static String wrapIfMissing(final String str, final String wrapWith) {
if (isEmpty(str) || isEmpty(wrapWith)) {
return str;
}
+
+ final boolean wrapStart = !str.startsWith(wrapWith);
+ final boolean wrapEnd = !str.endsWith(wrapWith);
+ if (!wrapStart && !wrapEnd) {
+ return str;
+ }
+
final StringBuilder builder = new StringBuilder(str.length() + wrapWith.length() + wrapWith.length());
Review comment:
Yeah, looks like this is the maximum capacity, not necessarily the exact capacity we expect to have. Probably harmless/fine?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] nstdio commented on a change in pull request #496:
Avoid unnecessary allocation in wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
nstdio commented on a change in pull request #496: Avoid unnecessary allocation in wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#discussion_r386031175
##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -9508,20 +9514,27 @@ public static String wrapIfMissing(final String str, final char wrapWith) {
* @param str
* the string to be wrapped, may be {@code null}
* @param wrapWith
- * the char that will wrap {@code str}
+ * the string that will wrap {@code str}
* @return the wrapped string, or {@code null} if {@code str==null}
* @since 3.5
*/
public static String wrapIfMissing(final String str, final String wrapWith) {
if (isEmpty(str) || isEmpty(wrapWith)) {
return str;
}
+
+ final boolean wrapStart = !str.startsWith(wrapWith);
+ final boolean wrapEnd = !str.endsWith(wrapWith);
+ if (!wrapStart && !wrapEnd) {
+ return str;
+ }
+
final StringBuilder builder = new StringBuilder(str.length() + wrapWith.length() + wrapWith.length());
Review comment:
I don't know if there is a practical reason to create `StringBuilder` with exact capacity, because it `str` can start with `wrapWith` but not end with it and vice versa.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] coveralls commented on issue #496: Avoid unnecessary
allocation in StringUtils.wrapIfMissing.
Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #496: Avoid unnecessary allocation in StringUtils.wrapIfMissing.
URL: https://github.com/apache/commons-lang/pull/496#issuecomment-592952175
[](https://coveralls.io/builds/29046434)
Coverage increased (+0.003%) to 95.104% when pulling **e4d937d63b5936ab4bc0ca80acd0d32cdc01457a on nstdio:optimize-wrap-if-missing** into **94b3784fdec5d0e9d63e4aec6772144b68283790 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services