Repository navigation
Require Julia 1.12 and fix memory-safety and failure-path problems - #59
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 97.31% 97.29% -0.03%
==========================================
Files 1 1
Lines 373 407 +34
==========================================
+ Hits 363 396 +33
- Misses 10 11 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Bump the version to 0.7.0, as several of these change behaviour. Julia 1.12 no longer sets up SuiteSparse when SparseArrays loads, so call init_suitesparse from _common, as its own solvers do on entry, rather than from __init__. Memory safety: - A factorization shares vectors with the caller, so dropzeros!(A) or resize! could leave K.nzval too short, and libklu read past its end. Operations that read these vectors now check the lengths first. Sharing stays, to avoid using more memory, and the docs now say what the caller must not do. - The pattern check in klu! shifted colptr and rowval in place, which other factorizations sharing them could see while inside libklu. - klu_analyze! validated the caller's permutations and then used them, so another task could change them in between. It now validates and uses copies. - Assigning the current libklu pointer freed it and left it dangling, and replacing the symbolic pointer left numeric factors that depended on it. - Fields that don't need to change are const, which leaves _symbolic, _numeric, _perms and nzval. Memory leak: - deserialize does not call the constructor, so a deserialized factorization had no finalizer, and never freed the libklu objects it made on demand. Register the finalizer when the first of them is attached instead. Failure paths: - When klu! stopped partway it left a mix of old and new factors, and a failed klu_factor! kept the earlier factors, so K \ b returned finite wrong answers. Both now release them, so later use throws until a factorization succeeds. - rcond and condest keep the status, as they could make issuccess return true. - An earlier failure no longer stops klu_factor! from retrying. - solve! with check=false doesn't throw when it factors on demand. - Check the result of klu_sort. - klu rejects empty matrices with a clear error, and KLU_INVALID has a clearer message. - SingularException reports a column with no pivot. Locking: - issuccess, nnz and show read state under the lock, and show prints factors from one factorization. Also: - klu_refactor! accepts keywords - add logabsdet, and so det and logdet, including for K' and transpose(K) - ldiv! accepts a single row with any column stride - K.symbolic and K.numeric, which hold raw pointers, are no longer listed as public - the docs for the scale setting describe rows rather than columns - remove the AdjointFact and TransposeFact aliases, and the unused `@isok` macro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ViralBShah
force-pushed
the
klu-owns-vectors-julia-1.12
branch
from
October 31, 2026 14:58
575be64 to
260e88d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows a review of the package for structure and soundness. This PR set the version to 0.7.0, but these changes are to be released as 0.6.1, in a follow-up that also stops exporting
solve!.Changes to the accepted element types are in #60, so
klu,klu!andldiv!accept the same types after this PR as before it.Behaviour changes
SparseArraysandLinearAlgebracompat move with it.Teststays at1.10, because that stdlib is still v1.11.0 on Julia 1.12 and 1.13.klu!orklu_factor!throws,K \ b,rcond,condestanddetthrow until a factorization succeeds._symbolic,_numeric,_permsandnzvalcan still be assigned.klu_factor!used to rethrow a stale error, even after a temporary lack of memory.solve!withcheck=falseno longer throws when it factors on demand.klurejects empty matrices with a clearArgumentError, asludoes.K.symbolicandK.numericare no longer listed bypropertynames(K), because they hold raw pointers into libklu's memory. They can still be accessed.Memory safety
dropzeros!(A)orresize!could leaveK.nzvaltoo short, and libklu read past its end. Operations that read these vectors now check the lengths first. Sharing itself is unchanged, to avoid using more memory, and the docs now say what the caller must not do.klu!shiftedcolptrandrowvalin place, which other factorizations sharing them could see while inside libklu.klu_analyze!validated the caller'sPandQ, then used them, so another task could change them in between. It now validates and uses copies._numericor_symbolicpointer freed it and left it dangling. Replacing_symbolicnow also releases the numeric factors that depend on it.Memory leak
deserializedoesn't call the constructor, so a deserialized factorization had no finalizer and never freed the libklu objects it made on demand. The finalizer is now registered when the first of them is attached.Peak memory growth over 60 rounds of solving with a 300,000 × 300,000 tridiagonal matrix and running GC:
Failure paths and locking
klu!stopped partway it left a mix of old and new factors, and a failedklu_factor!kept the earlier factors, soK \ breturned finite wrong answers.rcondandcondestcould then also makeissuccessreturntrue.issuccess,nnzandshownow read under the lock, andshowprints factors from one factorization.klu_sort: its result is now checked.klu_refactor!dropped keyword arguments.Other changes
init_suitesparseis now called from_commonrather than__init__, as the stdlib solvers do on entry.logabsdet, and sodetandlogdet, including forK'andtranspose(K).SingularExceptionreports a column with no pivot, andKLU_INVALIDhas a clearer message.ldiv!accepts a single row with any column stride.scalesetting now describe rows rather than columns.AdjointFactandTransposeFactaliases and the unused@isokmacro.Serializationas a test dependency.Testing
On Apple silicon macOS, the test suite passes on Julia 1.12.7 and 1.13.3, and Aqua and the docs build pass on 1.12.7. Linux and Windows are left to CI.
🤖 Generated with Claude Code