You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by GitBox <gi...@apache.org> on 2022/02/01 17:17:47 UTC
[GitHub] [wicket] theigl opened a new pull request #497: Performance improvements for `Strings.isEmpty()`
theigl opened a new pull request #497:
URL: https://github.com/apache/wicket/pull/497
This PR adds some micro-improvements for `Strings.isEmpty`.
`String.trim` is one of the hottest methods in my application. Most calls come from `Strings.isEmpty`, either called directly or via `Args.notEmpty`. It is called thousands of times during each request, so even minor improvements should make sense.
This PR applies two optimizations:
1. It adds overloaded methods for `Strings.isEmpty` and `Args.notEmpty` that take a `String` argument. The compiler seems to better inline the method when the exact parameter type is known at compile time. This results in about 5% performance improvement in most of my tests.
2. Before calling `String.trim`, the first character of the string is checked for whitespace. If it isn't whitespace, we can avoid calling `String.trim`. This optimization makes sense for the new method, but *especially* for the existing method with CharSequence parameter. One CharSequence frequently passed to `isEmpty` is `AppendingStringBuffer`. Before calling `trim` we have to convert it into a string with `toString`. In the case of `AppendingStringBuffer`, this *always* allocates a new string with the entire contents of the buffer.
```
Benchmark Mode Cnt Score Error Units
StringsBenchmark.bufferFastIsEmpty thrpt 15 4706409.686 ± 41865.767 ops/s
StringsBenchmark.bufferIsEmpty thrpt 15 4001127.728 ± 14525.221 ops/s
StringsBenchmark.fastIsEmpty thrpt 15 139232012.995 ± 1757089.132 ops/s
StringsBenchmark.isEmpty thrpt 15 130471674.653 ± 1604850.726 ops/s
```
The new method is about 15% faster for `AppendingStringBuffer` and about 5-10% faster for `String`.
The only case where these optimization will probably result in minor performance loss is for strings that actually start with whitespace. In that case we check the first character and then call trim. This shouldn't be a problem though because in 99.9% of cases the method is called with a null string, an empty string, or a "normal" string.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027288730
OK my previous benchmark was flawed because I created a new buffer for every run. The new `isEmpty` method for `AppendingStringBuffer´ is actually 40-50x faster for medium-sized buffers.
```
Benchmark Mode Cnt Score Error Units
StringsBenchmark.bufferFastIsEmpty thrpt 15 1918461027.579 ± 4949234.208 ops/s
StringsBenchmark.bufferIsEmpty thrpt 15 42567269.762 ± 40091.373 ops/s
StringsBenchmark.fastIsEmpty thrpt 15 1910306184.208 ± 4067311.978 ops/s
StringsBenchmark.isEmpty thrpt 15 1804225517.032 ± 1951669.787 ops/s
```
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027215114
And please create a JIRA issue - for the changelog
Thanks!
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027257846
Right, I'll add the JavaDoc and the ticket tomorrow!
> It would be good to have the benchmark code for future reference - either here as a comment or somewhere in the source tree
I suggest we add a separate Maven module `wicket-benchmarks`, where we can add this and future benchmarks. I have such a module on a branch, but had problems with the enforcer plugin complaining about the plugins I added to run the benchmarks from the command line. I need to play around with the plugin definitions and will create a separate PR if I can get it working. The main advantage is that you can checkout the sources on a (cloud) server and run the benchmarks there. For some performance optimizations in Kryo, I had benchmarks running for hours over night, to get the error rates as low as possible.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #497:
URL: https://github.com/apache/wicket/pull/497#discussion_r797322895
##########
File path: wicket-util/src/main/java/org/apache/wicket/util/lang/Args.java
##########
@@ -66,6 +66,26 @@
return argument;
}
+ /**
+ * Checks argument is not empty (not null and has a non-whitespace character)
+ *
+ * @param argument
+ * the argument to check for emptiness
+ * @param name
+ * the name to use in the error message
+ * @return The {@code argument} parameter if not empty
+ * @throws IllegalArgumentException
+ * when the passed {@code argument} is empty
+ */
Review comment:
`This method overloads {@link #isEmpty(CharSequence)} for performance reasons.` is enough.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl merged pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl merged pull request #497:
URL: https://github.com/apache/wicket/pull/497
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl removed a comment on pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl removed a comment on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027288730
OK my previous benchmark was flawed because I created a new buffer for every run. The new `isEmpty` method for `AppendingStringBuffer` is actually 40-50x faster for medium-sized buffers.
```
Benchmark Mode Cnt Score Error Units
StringsBenchmark.bufferFastIsEmpty thrpt 15 1918461027.579 ± 4949234.208 ops/s
StringsBenchmark.bufferIsEmpty thrpt 15 42567269.762 ± 40091.373 ops/s
StringsBenchmark.fastIsEmpty thrpt 15 1910306184.208 ± 4067311.978 ops/s
StringsBenchmark.isEmpty thrpt 15 1804225517.032 ± 1951669.787 ops/s
```
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #497:
URL: https://github.com/apache/wicket/pull/497#discussion_r796930980
##########
File path: wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java
##########
@@ -531,8 +531,24 @@ else if (aChar == 'f')
*/
public static boolean isEmpty(final CharSequence string)
{
- return (string == null) || (string.length() == 0) ||
- (string.toString().trim().length() == 0);
+ return string == null || string.length() == 0 ||
+ (string.charAt(0) <= ' ' && string.toString().trim().isEmpty());
+ }
+
+ /**
+ * Checks whether the <code>string</code> is considered empty. Empty means that the string may
+ * contain whitespace, but no visible characters.
+ *
+ * "\n\t " is considered empty, while " a" is not.
+ *
+ * @param string
+ * The string
+ * @return True if the string is null or ""
+ */
Review comment:
Same - please add a comment
##########
File path: wicket-util/src/main/java/org/apache/wicket/util/lang/Args.java
##########
@@ -66,6 +66,26 @@
return argument;
}
+ /**
+ * Checks argument is not empty (not null and has a non-whitespace character)
+ *
+ * @param argument
+ * the argument to check for emptiness
+ * @param name
+ * the name to use in the error message
+ * @return The {@code argument} parameter if not empty
+ * @throws IllegalArgumentException
+ * when the passed {@code argument} is empty
+ */
Review comment:
please add a comment why there are two "identical" methods.
in few months/years someone will wonder what is going on
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027300865
```
Benchmark Mode Cnt Score Error Units
StringsBenchmark.bufferFastIsEmpty thrpt 15 1918461027.579 ± 4949234.208 ops/s
StringsBenchmark.bufferIsEmpty thrpt 15 42567269.762 ± 40091.373 ops/s
```
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a change in pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #497:
URL: https://github.com/apache/wicket/pull/497#discussion_r797023488
##########
File path: wicket-util/src/main/java/org/apache/wicket/util/lang/Args.java
##########
@@ -66,6 +66,26 @@
return argument;
}
+ /**
+ * Checks argument is not empty (not null and has a non-whitespace character)
+ *
+ * @param argument
+ * the argument to check for emptiness
+ * @param name
+ * the name to use in the error message
+ * @return The {@code argument} parameter if not empty
+ * @throws IllegalArgumentException
+ * when the passed {@code argument} is empty
+ */
Review comment:
Should I simply add:
> This method overloads {@link #isEmpty(CharSequence)} for performance reasons.
Or should I speculate on why this is faster?
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027297383
https://issues.apache.org/jira/browse/WICKET-6952
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on pull request #497: Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027214721
It would be good to have the benchmark code for future reference - either here as a comment or somewhere in the source tree
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl edited a comment on pull request #497: WICKET-6952 Performance improvements for `Strings.isEmpty()`
Posted by GitBox <gi...@apache.org>.
theigl edited a comment on pull request #497:
URL: https://github.com/apache/wicket/pull/497#issuecomment-1027288730
OK my previous benchmark was flawed because I created a new buffer for every run. The new `isEmpty` method for `AppendingStringBuffer` is actually 40-50x faster for medium-sized buffers.
```
Benchmark Mode Cnt Score Error Units
StringsBenchmark.bufferFastIsEmpty thrpt 15 1918461027.579 ± 4949234.208 ops/s
StringsBenchmark.bufferIsEmpty thrpt 15 42567269.762 ± 40091.373 ops/s
StringsBenchmark.fastIsEmpty thrpt 15 1910306184.208 ± 4067311.978 ops/s
StringsBenchmark.isEmpty thrpt 15 1804225517.032 ± 1951669.787 ops/s
```
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org