-
Notifications
You must be signed in to change notification settings - Fork 99
std::tread and joint to remove threads to load #3582
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
base: main
Are you sure you want to change the base?
Conversation
6ee506d to
4487b95
Compare
nntrainer/models/neuralnet.cpp
Outdated
|
|
||
| for (auto &f : futures) | ||
| f.get(); | ||
| for (auto &t : threads) |
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.
for (auto& t : threads) {
if (t.joinable()) {
t.join();
}
}For avoid std::system_error, how about add check joinable() before join
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.
Thanks. fixed.
DonghakPark
left a comment
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!
This PR fixes load function using std::thread rather than futures. For windows, the threads created by futures are not removed and the total number of threads are increase. We fixes this by using std::thread and join. Signed-off-by: jijoong.moon <[email protected]>
| } else { | ||
| #if defined(_WIN32) | ||
| // Map per-task, then unmap immediately after: enables early release | ||
| // Map per-ask, then unmap immediately after: enables early release |
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.
Maybe typo? 'per-ask' -> 'per-task'
songgot
left a comment
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
djeong20
left a comment
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
Dependency of the PR
This PR fixes load function using std::thread rather than futures.
For windows, the threads created by futures are not removed and the
total number of threads are increase.
We fixes this by using std::thread and join.
signed-off-by jijoong.moon[email protected]