Skip to content

Fix #8291 parfor leak of redtoset variable - #8339

Merged
sklam merged 4 commits into
numba:mainfrom
sklam:fix/iss8291_parfor_leak
Mar 6, 2023
Merged

Fix #8291 parfor leak of redtoset variable#8339
sklam merged 4 commits into
numba:mainfrom
sklam:fix/iss8291_parfor_leak

Conversation

@sklam

@sklam sklam commented Aug 10, 2022

Copy link
Copy Markdown
Member

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

@DrTodd13 DrTodd13 left a comment

Copy link
Copy Markdown
Contributor

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

sklam commented Aug 17, 2022

Copy link
Copy Markdown
Member Author

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

sklam commented Aug 17, 2022

Copy link
Copy Markdown
Member Author

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

stuartarchibald commented Aug 19, 2022

Copy link
Copy Markdown
Contributor

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

@stuartarchibald

stuartarchibald commented Mar 6, 2023

Copy link
Copy Markdown
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
+1 more reviewer
@apmasell apmasell apmasell left review comments
Reviewers whose approvals may not affect merge requirements

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