Skip to content

Fix some leaks and races #1629

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
Merged
merged 3 commits into from
Nov 28, 2024
Merged

Fix some leaks and races #1629

merged 3 commits into from
Nov 28, 2024

Conversation

awni
Copy link
Member

@awni awni commented Nov 26, 2024 •
edited
Loading

  • Fixes 3 memory leaks because of not having objc pools in place :(
  • Fixes race condition setting wired and cache limits (needed a mutex)

Closes ml-explore/mlx-examples#1124

We should consider running our test suite with OBJC_DEBUG_MISSING_POOLS=YES and checking for leaks in the future as it is quite easy to introduce leaks between the C++ and objc interface. It doesn't quite run clean:

  • one of the PyTorch tests triggers it as well (e.g. no MLX involved)
  • we leak some stuff at program termination..

@ivanfioravanti
Copy link
Contributor

ivanfioravanti commented Nov 27, 2024 •
edited
Loading

I tested this for 24 hours in a row with mlx_lm.server and it works like a charm! Thanks

angeloskath
angeloskath approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@angeloskath angeloskath left a comment

Choose a reason for hiding this comment

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

🙏

@awni awni merged commit d4b222b into main Nov 28, 2024
5 checks passed
@awni awni deleted the fix_some_leaks branch November 28, 2024 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angeloskath angeloskath angeloskath approved these changes

@barronalex barronalex Awaiting requested review from barronalex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak in mlx_lm.server?
3 participants
@awni @ivanfioravanti @angeloskath