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

Fix memory leak in umfpack_numeric! #37472

Merged
merged 1 commit into from
Sep 10, 2020
Merged

Conversation

cfauchereau
Copy link
Contributor

Fix #37118
If umfpack_numeric! is called on a UmfpackLU struct that has already been factorized, its pointer "numeric" is overwritten without freeing the pointed Numeric object.

This simple fix does:

  • Check for an existing Numeric object before writing to UmfpackLU.numeric.
  • If it exists, free the object.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Just a quick style comment: indention is slightly off (by two characters?). These one-line branches are often written as

U.numeric != C_NULL && umfpack_free_numeric(U)

@cfauchereau
Copy link
Contributor Author

Just a quick style comment: indention is slightly off (by two characters?). These one-line branches are often written as

U.numeric != C_NULL && umfpack_free_numeric(U)

I made the change. It was a tab, my mistake.

@dkarrasch
Copy link
Member

Just to make sure: did you check that the performance benefits (of lu! over lu) are still preserved?

@cfauchereau
Copy link
Contributor Author

cfauchereau commented Sep 8, 2020

Just to make sure: did you check that the performance benefits (of lu! over lu) are still preserved?

lu! is still faster than lu. The benefit comes from the reuse of the Symbolic object, the Numeric object is recreated each time. reuse_numeric is a misleading variable name.

EDIT: reuse_numeric is fine, it is another optimization.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Thanks. I wonder if we ever manage to wrap these functions correctly.

@andreasnoack
Copy link
Member

Do we just ignore FreeBSD failures for the time being?

@dkarrasch dkarrasch merged commit b17cd10 into JuliaLang:master Sep 10, 2020
KristofferC pushed a commit that referenced this pull request Sep 10, 2020
@KristofferC KristofferC mentioned this pull request Sep 10, 2020
29 tasks
KristofferC pushed a commit that referenced this pull request Sep 10, 2020
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.

lu! leaks memory
4 participants