Skip to content

Fix #8291 parfor leak of redtoset variable #8339

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
Merged
merged 4 commits into from
Mar 6, 2023
Merged

Fix #8291 parfor leak of redtoset variable #8339

merged 4 commits into from
Mar 6, 2023

Conversation

sklam
Copy link
Member

@sklam sklam commented Aug 10, 2022

as titled.

@sklam sklam mentioned this pull request Aug 10, 2022
Closed
@sklam sklam marked this pull request as ready for review August 12, 2022 19:29
@sklam sklam requested a review from DrTodd13 as a code owner August 12, 2022 19:29
DrTodd13
DrTodd13 reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@DrTodd13 DrTodd13 left a comment

Choose a reason for hiding this comment

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

Maybe add a comment why this works (and is necessary) compared to why iterating through redarrs.values() doesn't work. Thanks for the patch.

@apmasell apmasell added this to the Numba 0.57 RC milestone Aug 16, 2022
@apmasell apmasell assigned apmasell and unassigned DrTodd13 Aug 16, 2022
@stuartarchibald stuartarchibald added the Effort - short Short size effort needed label Aug 16, 2022
apmasell
apmasell previously approved these changes Aug 16, 2022
View reviewed changes
@sklam
Copy link
Member Author

sklam commented Aug 17, 2022

Maybe add a comment why this works (and is necessary) compared to why iterating through redarrs.values() doesn't work. Thanks for the patch.

It's just that redtoset also needs releasing in additon to content in redarrs.values().

@sklam
Copy link
Member Author

sklam commented Aug 17, 2022

The notebook at https://gist.github.com/sklam/0d0c179518e89b8061cab8cbb650bcce is base on the reproducer in #8291 and I added plots to show the RSS memory usage

Before
Screen Shot 2022-08-17 at 10 04 02 AM

After
Screen Shot 2022-08-17 at 10 03 47 AM

There are still some occasional memory increase but the same is observe in the non-parallel version. I can't say for sure if there are still memory leak.

@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review labels Aug 17, 2022
@stuartarchibald
Copy link
Contributor

@DrTodd13 please could you take another look at this? Many thanks.

@stuartarchibald
Copy link
Contributor

@DrTodd13 thanks for reviewing, marking as ready to merge.

@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 Mar 6, 2023
@sklam sklam merged commit 4c1aef5 into numba:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrTodd13 DrTodd13 DrTodd13 approved these changes

@apmasell apmasell apmasell left review comments

Assignees

@apmasell apmasell

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.57 RC
Development

Successfully merging this pull request may close these issues.
4 participants
@sklam @stuartarchibald @apmasell @DrTodd13