-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
optimize: the high CPU usage issue of the rpcMergeMessageSend thread #6061
Conversation
…, anyway, sendSyncRequest will wake up the thread when it comes in and puts the message into the queue.2. tm and rm threads are merged, no need for a separate thread.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 2.x #6061 +/- ##
============================================
+ Coverage 49.55% 49.59% +0.04%
- Complexity 4759 4766 +7
============================================
Files 908 908
Lines 31362 31375 +13
Branches 3778 3782 +4
============================================
+ Hits 15540 15561 +21
+ Misses 14285 14279 -6
+ Partials 1537 1535 -2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请登记pr和作者信息至 https://github.com/seata/seata/tree/2.x/changes 中的2.x.md中
Please register the PR and author information in the 2.x.md file located at https://github.com/seata/seata/tree/2.x/changes
core/src/main/java/io/seata/core/rpc/netty/AbstractNettyRemotingClient.java
Outdated
Show resolved
Hide resolved
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. |
May I ask for your email address? I will send you my contact information so we can further communicate. |
@funky-eyes |
I have sent my contact information to this email address, please check it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance optimization PR needs sufficient data support.
in MergedSendRunnable synchronized block code . check queue's status first seems better, so it can be direct invoke wait. because synchronized is mutual exclusion,there would be two situation, first invoke notify, There must be data. so no wait happens. first invoke wait, then it will be awakened by notify. synchronized (mergeLock) {
try {
if (basketMap.values().stream().allMatch(Collection::isEmpty)) {
mergeLock.wait();
}
} catch (InterruptedException e) {
}
} |
Ok, I'll try to find data to support this optimization, thanks for the heads up! |
Very great idea! I will try to modify it this way and verify the feasibility |
@funky-eyes @slievrly @laywin Please help me to see the reasonableness of the test code Test ToolsPerformance test using JMH, CPU occupancy test using arthas Test Code
Test DataBefore thread mergingPerformance Testing Data8 thread 16 thread CPU Occupancy Testing DataAfter the thread mergePerformance Testing Data8 thread 16 thread CPU Occupancy Testing DataTest ConclusionThread merging has little effect on CPU utilization, but has a more significant effect on request delivery time, reducing request time by about 25%. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: /home/runner/work/seata/seata/core/src/main/java/io/seata/core/rpc/netty/AbstractNettyRemotingClient.java:347:17: 'if' is not followed by whitespace. [WhitespaceAround]
Error: /home/runner/work/seata/seata/core/src/main/java/io/seata/core/rpc/netty/AbstractNettyRemotingClient.java:347:118: '{' is not preceded with whitespace. [WhitespaceAround]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I have registered the PR changes.
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
#6041
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews