Skip to content
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

Jump to bottom

Solve memory leak to fix issue #7210 #7247

Merged
merged 12 commits into from Nov 29, 2021
Merged

Solve memory leak to fix issue #7210 #7247

merged 12 commits into from Nov 29, 2021

Conversation

ysheffer
Copy link
Contributor

@ysheffer ysheffer commented Jul 25, 2021 •
edited

Added a macro to _pymodule.h and set attributes in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c using a temporary variable to solve memory leak and fix #7210 .

@ysheffer
Modified _num_threads.c, omppool.cpp, tbbpool.cpp, workqueue.c to rem…
4e4005e 
…ove memory leaks.
@ysheffer
Added macro to _pymodule.h.
dea6ffb 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer
Added macro to _pymodule.h.
53bf06d 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer
Added macro to _pymodule.h.
66d82af 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer ysheffer closed this Jul 25, 2021
@ysheffer
Added macro to _pymodule.h.
32dae68 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer
Added macro to _pymodule.h.
1c5a277 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer ysheffer reopened this Jul 25, 2021
@ysheffer
Added macro to _pymodule.h.
a0a6ffe 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer
Added macro to _pymodule.h.
c573472 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@esc
Copy link
Member

esc commented Jul 26, 2021

@ysheffer thank you for submitting this to the Numba issue tracker. It seems like your PR is doing a lot of cosmetics to the code in addition to implementing the feature. Do you think you could submit this PR with only the feature and not the cosmetics, as this makes it harder to review. Thanks!

@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 26, 2021
@ysheffer
Added macro to _pymodule.h.
882026c 
Redefined MOD_INIT sections in _num_threads.c, _omppool.cpp,
tbbpool.cpp, workqueue.c.
@ysheffer
Copy link
Contributor Author

@esc Thank you for your comment. Sure thing, I have updated the PR accordinaglly.

@esc esc added 3 - Ready for Review Effort - short Short size effort needed and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 26, 2021
@esc
Copy link
Member

esc commented Jul 26, 2021

@ysheffer thank you for making those changes, I have added it to the queue for review.

Snape3058
Snape3058 reviewed Jul 27, 2021
View reviewed changes
numba/_pymodule.h Outdated
@@ -24,5 +24,10 @@
#define PyInt_Type PyLong_Type
#define PyInt_Check PyLong_Check
#define PyInt_CheckExact PyLong_CheckExact
// Define object attribute using a temporary variable to avoid memory leak
#define SetAttrStringFromVoidPointer(m, name, tmp) \
Copy link

Choose a reason for hiding this comment

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

I think you had better wrap these three lines with a do { } while (false) block and declare a temporary variable in the block instead of using an external variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Snape3058 thanks, agree.

Copy link
Member

Choose a reason for hiding this comment

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

This is now applied, but I used while (0) instead of while (false) because false is not defined everywhere the macro is used.

stuartarchibald
stuartarchibald reviewed Aug 17, 2021
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

@ysheffer Many thanks for the patch, couple of minor things to look at else looks good.

numba/_pymodule.h Outdated
@@ -24,5 +24,10 @@
#define PyInt_Type PyLong_Type
#define PyInt_Check PyLong_Check
#define PyInt_CheckExact PyLong_CheckExact
// Define object attribute using a temporary variable to avoid memory leak
#define SetAttrStringFromVoidPointer(m, name, tmp) \
Copy link
Contributor

Choose a reason for hiding this comment

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

@Snape3058 thanks, agree.

numba/np/ufunc/omppool.cpp Outdated Show resolved Hide resolved
numba/_pymodule.h Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Aug 17, 2021
@stuartarchibald stuartarchibald added this to the Numba 0.55 RC milestone Nov 18, 2021
sklam and others added 2 commits November 18, 2021 09:13
@sklam @stuartarchibald
Apply suggestions from code review
df9fc50 
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@gmarkall
Wrap SetAttrStringFromVoidPointer in do { } while (0)
9695400 
This avoids it needing to have a temp passed in.

Formatting is also altered slightly to match that of existing code.
@gmarkall
Copy link
Member

@stuartarchibald This should be ready for another look now.

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 25, 2021
stuartarchibald
stuartarchibald approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

@ysheffer Many thanks for the patch, @gmarkall many thanks for responding to comments and providing fixes against the original patch. Good to see this leak patched.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 25, 2021
@stuartarchibald
Copy link
Contributor

@ysheffer Congratulations on your first contribution to Numba!

@sklam sklam merged commit 953b087 into numba:master Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarkall gmarkall gmarkall left review comments

@Snape3058 Snape3058 Snape3058 left review comments

@stuartarchibald stuartarchibald stuartarchibald approved these changes

@esc esc Awaiting requested review from esc

@sklam sklam Awaiting requested review from sklam

Assignees

@gmarkall gmarkall

Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed
Projects
None yet
Milestone
Numba 0.55 RC
Development

Successfully merging this pull request may close these issues.

Memory leak bugs in module init functions with PyObject_SetAttrString
6 participants
@ysheffer @esc @gmarkall @stuartarchibald @Snape3058 @sklam