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

Address memory leak in overloaded operators #297

Merged
merged 14 commits into from
Feb 27, 2025
Merged

Conversation

jwallwork23
Copy link
Collaborator

@jwallwork23 jwallwork23 commented Feb 26, 2025

Closes #295.

This PR makes torch_tensor_delete the destructor for the torch_tensor class. This means that it gets called automatically when a tensor goes out of scope. I've set it up such that the user may still call the subroutine explicitly if they wish, but this is no longer required.

I also changed the overloaded operators so that inputs and outputs to the C++ functions are all pointers, meaning that only the empty/zeros/ones/blob/array constructors create new torch::Tensor objects on the C++ side. The overloaded operators now merely operate on values for the data associated with the tensors they point to.

@jwallwork23 jwallwork23 added bug Something isn't working autograd Tasks towards the online training / automatic differentiation feature labels Feb 26, 2025
@jwallwork23 jwallwork23 self-assigned this Feb 26, 2025
@jwallwork23 jwallwork23 marked this pull request as ready for review February 26, 2025 17:35
@jwallwork23
Copy link
Collaborator Author

Since the current best practice still works fine, I think we can defer mentioning the destructor here and put some text on that in the upcoming tensor docs in #291.

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Yep, we discussed at length together yesterday and located the nullification together so I am happy with the concepts and approach.

A couple of non-blocking queries so happy for this to merge when ready.

Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming test_tensor_constructors_destructors.pf?
Also I get why you test empty and delete in a single subroutine but would it be relatively straightforward to separate them to try and keep things monotised (is that a word...?)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 0ac7b35 and b6042cb.

@jwallwork23 jwallwork23 merged commit dcdadef into main Feb 27, 2025
4 of 5 checks passed
@jwallwork23 jwallwork23 deleted the 295_oo-mem-leak branch February 27, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autograd Tasks towards the online training / automatic differentiation feature bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overloaded operators don't deallocate memory
2 participants