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.

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

Introduce Storage::forget_elements() to fix memory leak in Matrix::generic_resize() #1379

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Introduce Storage::forget_elements() to fix memory leak in Matrix::generic_resize() #1379

wants to merge 5 commits into from

Conversation

nickmertin
Copy link

Fixes #1378

Adds a trait method Storage::forget() to clean up the storage while forgetting (i.e., not dropping) the contents. Makes use of this method instead of mem::forget() in the implementations of Reallocator for DefaultAllocator.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 10, 2024

Thanks, this is great! I don't have the head space or time to wrap my head around the nuances of the Reallocator API here at the moment, but I wanted to ask if the documentation needs to be updated to reflect any possible changes in assumptions made. I don't fully understand them yet myself, I'd need a closer look later to try to verify.

@nickmertin
Copy link
Author

Thanks! I looked over the documentation and I don't think any changes are necessary, as it maintains the assumption that the elements of the matrix are not dropped. My only question would be whether it would make more sense to move the new method to RawStorage instead of Storage.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented May 6, 2024

I haven't forgotten this PR, but I haven't yet been able to devote time to it. It came up on Discord though, and @sebcrozet suggested that the forget function needs to be unsafe, and maybe it should be called forget_elements or something along those lines.

I'll try to get back to this soon, but I suspect I'll be quite busy this week.

@nickmertin
Copy link
Author

I like the name forget_elements, I think it gets the behaviour across more accurately than what I put right now.

I don't think it should be unsafe, for the same reason that mem::forget isn't unsafe. Safety does not guarantee that destructors are called, so there is nothing unsafe about calling this function. Marking it as unsafe would push obligations of ensuring safety onto everywhere it is called, which could be hard to follow given that it is a trait method and there are several implementations. Perhaps the behaviour and safety attributes just need to be better documented, as is the case for mem::forget.

tpdickso
tpdickso approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@tpdickso tpdickso left a comment •
edited

Choose a reason for hiding this comment

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

This looks appropriate to me! And I agree, I think a name like forget_elements would be great to help highlight that it's the elements that are being forgotten and not the storage itself (which will be dropped normally absent any other intervention.)

Andlon
Andlon requested changes May 9, 2024
View reviewed changes
Copy link
Sponsor Collaborator

@Andlon Andlon left a comment

Choose a reason for hiding this comment

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

You're absolutely right about the fact that forget doesn't have to be unsafe @nickmertin! I just have a few minor suggestions, then we can get this merged in time for the next release :)

src/base/storage.rs Outdated Show resolved Hide resolved
src/base/vec_storage.rs Outdated Show resolved Hide resolved
@Andlon Andlon added the patch version Merging PR only requires patch version bump label May 9, 2024
@nickmertin
Copy link
Author

Sounds good, I will make the necessary changes this weekend.

@nickmertin nickmertin changed the title Introduce Storage::forget() to fix memory leak in Matrix::generic_resize() Introduce Storage::forget_elements() to fix memory leak in Matrix::generic_resize() May 12, 2024
@nickmertin
Copy link
Author

I believe it is now good to go, let me know if any other changes are needed!

@Andlon Andlon added breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes minor version Merging PR requires minor version bump merge ready PR is ready for merging and removed patch version Merging PR only requires patch version bump labels May 13, 2024
@Andlon
Copy link
Sponsor Collaborator

Andlon commented May 13, 2024

Thanks @nickmertin, it's ready to merge now. Great to have this bug fixed! I'm not merging immediately because of the breaking change in introducing forget_elements to the Storage trait. In principle, I think we could merge directly since the dev branch already contains unreleased breaking changes, but I'll run it by the others later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andlon Andlon Andlon requested changes

@tpdickso tpdickso tpdickso approved these changes

Assignees
No one assigned
Labels
breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes merge ready PR is ready for merging minor version Merging PR requires minor version bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak in generic_resize/reallocate_copy
3 participants
@nickmertin @Andlon @tpdickso