Tweak pollTimedOutRPCs thread synchronization #30355
Closed
Conversation
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
- Make processTimedOutFutures hold lock - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
rohan-varma
reviewed
Nov 23, 2019
rohan-varma
reviewed
Nov 23, 2019
rohan-varma
reviewed
Nov 23, 2019
rohan-varma
reviewed
Nov 23, 2019
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
rohan-varma
reviewed
Nov 23, 2019
rohan-varma
reviewed
Nov 23, 2019
xush6528
changed the title
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Nov 23, 2019
Pull Request resolved: #30355 - Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. ghstack-source-id: 94476372 Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/)
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Nov 24, 2019
Pull Request resolved: #30355 - Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. ghstack-source-id: 94487097 Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/)
xush6528
added
the
module: rpc
mrshenli
reviewed
Dec 10, 2019
Contributor
mrshenli
left a comment
mrshenli
left a comment
There was a problem hiding this comment.
This LGTM. Do we know why we hit failures in MacOS test?
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
changed the title
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Dec 11, 2019
Pull Request resolved: #30355 - Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. ghstack-source-id: 95326061 Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/)
xush6528
changed the title
jjlilley
reviewed
Dec 11, 2019
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
Member
CircleCI build failures summaryAs of commit e29a7c8:
Detailed failure analysisOne may explore the probable reasons each build failed interactively on the Dr. CI website. 6 new failures recognized by patternsThe following build failures don't appear to be due to upstream breakage:
|
| Job | Step |
|---|---|
 caffe2_onnx_py3_6_clang7_ubuntu16_04_build |
Build |
This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.
Please report bugs/suggestions on the GitHub issue tracker.
This comment has been revised 2 times.
- Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/) [ghstack-poisoned]
xush6528
added a commit
that referenced
this pull request
Dec 12, 2019
Pull Request resolved: #30355 - Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. ghstack-source-id: 95409528 Differential Revision: [D5516149](https://our.internmc.facebook.com/intern/diff/D5516149/)
rohan-varma
reviewed
Dec 12, 2019
|
|
||
| void ProcessGroupAgent::pollTimedOutRPCs() { | ||
| while (true) { | ||
| while (rpcRunning_.load()) { |
Contributor
There was a problem hiding this comment.
Don't we need to hold the lock here while checking rpcRunning? That's why there was the while(true) before
Contributor
Author
There was a problem hiding this comment.
The lock was to avoid ::shutdown() noitifying when the watchdog thread is not waiting on the cond var, thus avoid the watchdog thread missing the notification.
With the predicate, we can allow the watch dog missing the notification.
Even if the watch dog thread misses the notification, the predicate will evaluate to false (since rpcRunning_ has already been set to False), the thread will not wait on the cond var, and gets back to the loop condition and decides to exit the loop.
rohan-varma
reviewed
Dec 12, 2019
| if (!rpcRunning_.load()) { | ||
| return; | ||
| if (shouldUpdateMinEndTime) { | ||
| continue; |
Contributor
There was a problem hiding this comment.
With this continue, I guess this means processing of the timed out futures is going to be skipped until we do another round of updating the minEndTime? If there is a timed out future, then there will be some delay in processing it, do we need to have this continue?
Contributor
Author
There was a problem hiding this comment.
Sorry, I thought we are closed on #30355 (comment).
Two conditions it will stop the thread from waiting on the cond var.
-
The cond var is notified. It means there is an earlier
endTimekey inserted into the map.
So it will go back to the beginning of the loop, update the thread-localminEndTimeand- if
now >= the latest minEndTime, it will process the futures immediately without waiting on the cond var. - if
now < the latest minEndTime, it will wait untilminEndTime
- if
-
Time is up.
now >= the latest minEndTime. It will wake from the wait and and process the timed out futures.
Contributor
|
This pull request has been merged in 2488231. |
wuhuikx
pushed a commit
to wuhuikx/pytorch
that referenced
this pull request
Jan 30, 2020
Summary: Pull Request resolved: pytorch#30355 - Make processTimedOutFutures hold lock. - Reduce unnecessary scan on future and future timeout maps. - Reduce the scope of lock at a spot. - Avoid repeatedly wake up if user set timeout = 0. ghstack-source-id: 95409528 Test Plan: # Unit tests ``` buck test mode/dev-nosan //caffe2/test:rpc_fork -- test_rpc_timeouts buck-out/gen/caffe2/test/rpc_fork\#binary.par -r test_rpc_timeouts ``` ``` buck test mode/dev-nosan //caffe2/test:rpc_fork_thrift -- test_rpc_timeouts buck-out/gen/caffe2/test/rpc_fork_thrift\#binary.par -r test_rpc_timeouts ``` Differential Revision: D5516149 fbshipit-source-id: 4bb0bd59fa31d9bfaef9f07ac0126782da17f762
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Stack from ghstack:
Differential Revision: D5516149