Simplify string concatenation in log#1378
Merged
chickenlj merged 1 commit intoapache:masterfrom Feb 24, 2018
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1378 +/- ##
==========================================
- Coverage 32.57% 32.37% -0.21%
==========================================
Files 691 691
Lines 34537 34534 -3
Branches 6812 6812
==========================================
- Hits 11252 11180 -72
- Misses 21357 21419 +62
- Partials 1928 1935 +7
Continue to review full report at Codecov.
|
kimmking
reviewed
Feb 20, 2018
| .append("Call Decodeable.decode failed: ") | ||
| .append(e.getMessage()).toString(), | ||
| e); | ||
| log.warn("Call Decodeable.decode failed: " + e.getMessage(), e); |
Member
There was a problem hiding this comment.
the capacity of StringBuilder in modified statement is 16, not equals with 32 in the original one
Contributor
Author
There was a problem hiding this comment.
Thanks @kimmking , what do you think about this change?
Contributor
There was a problem hiding this comment.
I think this is a good optimization, let's wait for other suggestions before merge.
Contributor
Author
There was a problem hiding this comment.
Thanks @chickenlj . For those cases, the naive summation will be much faster than StringBuilder. I made a simple test:
public static void main(String[] args) {
for (int t = 0; t < 10; t++) {
long st = System.currentTimeMillis();
for (int i = 0; i < 100000; i++) {
String s = "Decode decodeable message " + "xxx";
}
long e = System.currentTimeMillis();
System.out.print(e - st);
System.out.print(" ");
st = System.currentTimeMillis();
for (int i = 0; i < 100000; i++) {
new StringBuilder(32).append("Decode decodeable message ").append("xxx").toString();
}
e = System.currentTimeMillis();
System.out.println(e - st);
}
}
and I got output:
1 13
0 11
0 4
0 3
0 4
0 4
0 4
0 1
0 1
0 1
Member
There was a problem hiding this comment.
this is not an equivalent comparison. the compiler may optimize "Decode decodeable message " + "xxx" into "Decode decodeable message xxx" directly.
Contributor
Author
There was a problem hiding this comment.
Thanks @beiwei30 ! That's my mistake. The test is uncorrect. The two strings cannot both be constants. 👍
zonghaishang
pushed a commit
to zonghaishang/dubbo
that referenced
this pull request
Feb 26, 2018
* @reference support annotate on annotation type * Fixes apache#1303 TimeUnit conversion error * Fixes apache#1289, use bind_port as mapping key * Fixes apache#1313, remove destroy check in Registry. * checkout .travis.yml from origin/master * Fix hessian2 serialized short, byte is converted to int bug (apache#1232) * Fix hessian2 serialized short, byte is converted to int bug * Fix hessian2 serialized short, byte is converted to int bug * adapt jdk1.5+ * fixed travis-ci failed because of test cases. (apache#1370) * Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable() * Merge pull request apache#1378, replace StringBuider with simple string concatenation in log. * Merge pull request apache#1375, remove unnecessary boxing. Fixes apache#1245 * Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version. * Merge pull request apache#1376, do not instantiate load balance if there is no invokers Fixes apache#1297 * Merge pull request apache#1384, fix build string bug. * Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis. * Merge pull request apache#1242, remove redundant null check. fixes apache#1231
zonghaishang
pushed a commit
to zonghaishang/dubbo
that referenced
this pull request
Feb 26, 2018
* @reference support annotate on annotation type * Fixes apache#1303 TimeUnit conversion error * Fixes apache#1289, use bind_port as mapping key * Fixes apache#1313, remove destroy check in Registry. * checkout .travis.yml from origin/master * Fix hessian2 serialized short, byte is converted to int bug (apache#1232) * Fix hessian2 serialized short, byte is converted to int bug * Fix hessian2 serialized short, byte is converted to int bug * adapt jdk1.5+ * fixed travis-ci failed because of test cases. (apache#1370) * Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable() * Merge pull request apache#1378, replace StringBuider with simple string concatenation in log. * Merge pull request apache#1375, remove unnecessary boxing. Fixes apache#1245 * Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version. * Merge pull request apache#1376, do not instantiate load balance if there is no invokers Fixes apache#1297 * Merge pull request apache#1384, fix build string bug. * Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis. * Merge pull request apache#1242, remove redundant null check. fixes apache#1231 * Change Mailing list address * Fixed apache#1398, revert bugs introduced from apache#1375 * Fix time unit problem in UT * Fix time unit problem related with FutureAdapter in UT
zonghaishang
pushed a commit
to zonghaishang/dubbo
that referenced
this pull request
Mar 2, 2018
* @reference support annotate on annotation type * Fixes apache#1303 TimeUnit conversion error * Fixes apache#1289, use bind_port as mapping key * Fixes apache#1313, remove destroy check in Registry. * checkout .travis.yml from origin/master * Fix hessian2 serialized short, byte is converted to int bug (apache#1232) * Fix hessian2 serialized short, byte is converted to int bug * Fix hessian2 serialized short, byte is converted to int bug * adapt jdk1.5+ * fixed travis-ci failed because of test cases. (apache#1370) * Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable() * Merge pull request apache#1378, replace StringBuider with simple string concatenation in log. * Merge pull request apache#1375, remove unnecessary boxing. Fixes apache#1245 * Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version. * Merge pull request apache#1376, do not instantiate load balance if there is no invokers Fixes apache#1297 * Merge pull request apache#1384, fix build string bug. * Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis. * Merge pull request apache#1242, remove redundant null check. fixes apache#1231 * Change Mailing list address * Fixed apache#1398, revert bugs introduced from apache#1375 * Fix time unit problem in UT * Fix time unit problem related with FutureAdapter in UT * Fix time unit problem related with FutureAdapter in UT * Merge pull request apache#1391, fix typo of method name in qos module. * fix hessian lite test case fail bug (apache#1394) * fix hessian lite test case fail bug * update test * remove ignore * Fix time unit problem related with FutureAdapter in UT * revert file * fix number type is lost in yaml config file (apache#1401) * apache#1399 fi * update test * update readme to add some details (apache#1403) * update readme to add some details update readme to add some details * delete duplicated words delete duplicated words * update README format * apache#1411: Locale deserialize 'zh-hant_CN'
rolandhe
pushed a commit
to rolandhe/dubbo
that referenced
this pull request
Sep 9, 2019
…ng concatenation in log.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We can simplify string concatenation in log without
StringBuildercause a single naive string concatenation cost is also cheap.