-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-17749. Remove lock contention in SelectorPool of SocketIOWithTimeout #3080
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Could someone please review this PR? |
@jojochuang Can you kindly review this PR? |
I tested the performance of the trunk version and the optimized version. Test CaseThe test steps are as follow:
Code project: https://github.com/liangxs/test-HADOOP-17749 Test ResultThe test result is as follow:
ps, I used two test machines with same hardware and software configuration and on a same rack:
|
@liangxs Thanks for contribution, performance looks pretty good. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ferhui Thanks for your suggestion, I added unit test. |
if (!trimming.compareAndSet(false, true)) { | ||
return; | ||
} | ||
if (now - lastTrimTime < IDLE_TIMEOUT / 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.
Why is it IDLE_TIMEOUT/2 here?
Maybe idle time is near 1.5 times IDLE_TIMEOUT in extreme cases。
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.
@ferhui I remove this check in new commit.
And please see the checkstyle reports |
💔 -1 overall
This message was automatically generated. |
@aajisaka @jojochuang Could you please help review this? performance improvement looks good |
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.
The patch mostly looks good to me. Thank you @liangxs.
...op-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
Outdated
Show resolved
Hide resolved
...op-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
…cketIOWithTimeout (apache#3080) (cherry picked from commit a5db683) Change-Id: Ief8d9eb2700a340f2dea0a9fd4c1e8791bdce37c (cherry picked from commit 753eb1766eb67760d058a17ff31c399eaf2edd2a)
JIRA: HADOOP-17749