Ensure thread id is valid in nested parallel regions #60183
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
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 6677a6e (more details on the Dr. CI page and at hud.pytorch.org/pr/60183):
🕵️ 4 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
| Job | Step | Action |
|---|---|---|
 pytorch_linux_bionic_py3_6_clang9_noarch_test |
Report results | 🔁 rerun |
1 job timed out:
pytorch_linux_bionic_py3_6_clang9_noarch_test
This comment was automatically generated by Dr. CI (expand for details).
Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group.
This was referenced Jun 17, 2021
peterbell10
added
module: openmp
|
Does only OpenMP need changes here? What about other parallel backends? No changes needed, but it's a pattern that you've eliminated recently in reduction, so now it became slightly more expensive. |
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
I think your right, I've made similar changes to the other backends as well. |
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. [ghstack-poisoned]
|
Great, can you please add a test for it (probably cpp test)? |
|
@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Fixes #59149 (comment) `parallel_for` will call the function directly if it would have run on only a single thread anyway. This is great for performance, but causes an issue in nested parallel regions because `get_thread_num` will reflect the parent parallel region instead of the current `parallel_for` call. I fix this by using a `thread_local` variable for the current thread id and manually setting it before each call to the user-provided function. Differential Revision: [D29287816](https://our.internmc.facebook.com/intern/diff/D29287816) [ghstack-poisoned]
peterbell10
commented
Jun 22, 2021
| TEST(TestParallel, TestParallel) { | ||
| manual_seed(123); | ||
| set_num_threads(1); | ||
| NumThreadsGuard guard(1); |
There was a problem hiding this comment.
This test was having the global side-effect of disabling multi-threading.
ngimel
approved these changes
Jun 22, 2021
|
@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Stack from ghstack:
Fixes #59149 (comment)
parallel_forwill call the function directly if it would have run on only asingle thread anyway. This is great for performance, but causes an issue in
nested parallel regions because
get_thread_numwill reflect the parentparallel region instead of the current
parallel_forcall.I fix this by using a
thread_localvariable for the current thread id andmanually setting it before each call to the user-provided function.
Differential Revision: D29287816