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

[Memory Leak] Revert "[TE][Fix] Comparison of the output tensor" #10540

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

AndrewZhaoLuo
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo commented Mar 9, 2022

Reverts #9829

The above behavior avoids having TE output Tensors change in address everytime by caching the tensors instead of reallocating everytime. However in doing so they introduce a cyclic dependency. Each Operation has a list of Tensors. However, each Tensor has a reference to the Operation. As a result, each will always have a reference to each other and never be deleted under TVM's reference counting system.

Possible alternatives:

  • Something like a weak_ptr, probably so a counted reference exists between the tensor --> operator but not in the opposite direction. Don't know if this is in TVM rn
  • Refactoring above PR to somehow avoid cyclic dependency

I'm reverting for now though.

Thanks to @mbrookhart for help in finding this memory error!

@AndrewZhaoLuo AndrewZhaoLuo changed the title Revert "[TE][Fix] Comparison of the output tensor" [Memory Leak] Revert "[TE][Fix] Comparison of the output tensor" Mar 9, 2022
@mbrookhart
Copy link
Contributor

Cc @leeexyz @Hzfengsy @junrushao1994

@tqchen
Copy link
Member

tqchen commented Mar 9, 2022

I agree that the PR should be revereted.

To compare two tensors, we need to use deep comparison, the operator== of Tensor is also overloaded. In general we should avoid self-reference here

@tqchen
Copy link
Member

tqchen commented Mar 9, 2022

cc @leeexyz

@leeexyz
Copy link
Contributor

leeexyz commented Mar 9, 2022

@AndrewZhaoLuo Sorry for the issue I introduced that I did not think too much. I am just wondering the original idea here is also has a cyclic dependency, operation <-> tensor, even the tensors are created every time.

@tqchen I agree with your suggestion that using Tensor::operator== is a better solution.

Thanks for your kind help.

@Hzfengsy
Copy link
Member

Hzfengsy commented Mar 9, 2022

I'm sorry that I didn't find the problem during reviewing. Please fix the CI and merge it as soon as possible.

@AndrewZhaoLuo
Copy link
Contributor Author

@leeexyz the old way tensor has a reference to the operator but operator does not have a reference to the tensor so we do not have cyclic references.

@masahi masahi merged commit ffd5f70 into main Mar 9, 2022
ziqiangxu8457 pushed a commit to ziqiangxu8457/tvm that referenced this pull request Mar 9, 2022
@tqchen tqchen deleted the revert-9829-te_fix branch March 10, 2022 13:53
ziqiangxu8457 pushed a commit to ziqiangxu8457/tvm that referenced this pull request Mar 12, 2022
pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants