- Notifications
You must be signed in to change notification settings - Fork26.3k
[AOTI] Fix a memory leak in model_package_loader#152334
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pytorch-botbot commentedApr 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
🔗 Helpful Links🧪 See artifacts and rendered test results athud.pytorch.org/pr/152334
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commitb80c6a1 with merge base7ce6f63 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was present on the merge base:👉Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
angelayi left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm kind of surprised that CI doesn't have some sort of valgrind test
Skylion007 commentedApr 28, 2025
@pytorchbot merge |
pytorchmergebot commentedApr 28, 2025
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in thewiki. Questions? Feedback? Please reach out to thePyTorch DevX Team |
pytorchmergebot commentedApr 28, 2025
Merge failedReason: 2 mandatory check(s) failed. The first few are: Dig deeper byviewing the failures on hud |
Summary: There was a char array allocated but never freed. It was found by valgrind and verified fixed with this PR, although it's not easy to write a unit test for it.
…n't want to spend more time to fight it.
| } | ||
| } | ||
| } | ||
| delete[] filename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This could still memory leak on exception right? Can't we just put it in a unique_ptr with a proper user defined destructor?
Skylion007Apr 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or better yet, why not just stack allocate the array? Why both heap allocating it all if it's just deleted when the stack is cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
By stack allocation, do you mean Variable Length Array? I don't think that's supported by MSVC?
I can dig more with the previous std::string approach.
desertfire commentedApr 30, 2025
@pytorchbot merge |
pytorchmergebot commentedApr 30, 2025
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in thewiki. Questions? Feedback? Please reach out to thePyTorch DevX Team |
Summary: There was a char array allocated but never freed. It was found by valgrind and verified fixed with this PR, although it's not easy to write a unit test for it.