Skip to content

update mkldnn-bridge to avoid mem leak #22392

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.

Sign up for GitHub

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

Jump to bottom
Closed
gujinghui wants to merge 1 commit into from
Closed

update mkldnn-bridge to avoid mem leak #22392

gujinghui wants to merge 1 commit into from

Conversation

@gujinghui
Copy link
Collaborator

@gujinghui gujinghui commented Jul 1, 2019

fix the memory leak issue exposed by #21537

@gujinghui gujinghui mentioned this pull request Jul 1, 2019
Closed
@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 1, 2019

@Jianhui-Li @uyongw @XiaobingSuper @jgong5

@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 1, 2019

@bddppq @yinghai
pls help review.

@mrshenli mrshenli mentioned this pull request Jul 1, 2019
Closed
@bddppq
Copy link
Contributor

bddppq commented Jul 1, 2019

@gujinghui Thanks for the fix!
Could you elaborate a little bit about the fix? (e.g. where were the threads created? Is default_stream still thread-safe after this change?)

facebook-github-bot
facebook-github-bot reviewed Jul 1, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bddppq has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@bddppq bddppq added the module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration label Jul 1, 2019
@bddppq
Copy link
Contributor

bddppq commented Jul 1, 2019

foo

@uyongw
Copy link

uyongw commented Jul 2, 2019

Could you elaborate a little bit about the fix? (e.g. where were the threads created? Is default_stream still thread-safe after this change?)

It is still thread-safe because stream is now a temporal stack variable, instead of a static thread-local storage that can hold internal state. That internal state may cause memory bloat. @bddppq

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 2, 2019

@bddppq merged this pull request in cbf5726.

xzhu1900 pushed a commit to xzhu1900/pytorch that referenced this pull request Jul 5, 2019
@gujinghui @xzhu1900
update mkldnn-bridge to avoid mem leak (pytorch#22392)
11d1fdc 
Summary:
fix the memory leak issue exposed by pytorch#21537
Pull Request resolved: pytorch#22392

Test Plan: {F164886124}

Reviewed By: yinghai

Differential Revision: D16074150

Pulled By: bddppq

fbshipit-source-id: b70192aad3d531f349fea5d2d477b827715a2363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

+1 more reviewer

@bddppq bddppq bddppq approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration module: third_party open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gujinghui @bddppq @uyongw @facebook-github-bot @ezyang @pytorchbot @mruberry