Use hasmethod for checking mul! implementation#406
Use hasmethod for checking mul! implementation#406amontoison merged 4 commits intoJuliaSmoothOptimizers:mainfrom
hasmethod for checking mul! implementation#406Conversation
The `LinearOperator` constructor was insanely slow (~5μs) because of the call to `get_nargs`. This removes that call and much of the 5-arg logic, replacing with the use of `hasmethod`. Because all the types passed to `hasmethod` are concrete, Julia's compiler can decide this at compile time, so there is no actual runtime cost to this check. This also removes the internal storage for `has_args5` since we can't compute that at construction time without making the constructor very slow. One consequence is a limited test breakage: `has_args5` now refers to the "surface-level" operator only, and formerly some composite operations (e.g., `hcat`, `+`, etc) would "lie" about whether there was a 5-arg `mul!` available for the composite object in favor of reporting whether that was true for all the constituents. With this change, `has_args5` will report whether the composite object itself has a 5-arg `mul!` available, which in cases here is "yes." But a few tests were checking that the composite object reported "no". In addition to this change, the fields of `mutable struct`s that do not seem to be intended to be mutated have been made `const`. This should also result in some performance improvements. Fixes JuliaSmoothOptimizers#275 Closes JuliaSmoothOptimizers#282
There was a problem hiding this comment.
Pull request overview
This PR targets performance regressions in LinearOperator construction by removing the expensive get_nargs-based constructor logic and switching runtime behavior to prefer hasmethod checks for 5-arg mul! support, while also tightening operator structs by making non-mutated fields const.
Changes:
- Reworked 5-arg
mul!selection to usehasmethodchecks and removed most of the prior “5-arg capability” bookkeeping from composite operator construction. - Simplified composite-operator constructors (
+,*,hcat/vcat,BlockDiagonalOperator, etc.) to directly returnLinearOperator{T,S}(...)without propagatingargs5. - Made many fields in
mutable structsconstto improve performance and compiler reasoning.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/abstract.jl |
Removes stored args5/allocation flags from LinearOperator, adds lazy buffers Mv/Mtu, and updates has_args5/isallocated5 plumbing. |
src/operations.jl |
Switches 5-arg mul! dispatch to hasmethod, updates buffer allocation path for 2-arg prod!. |
src/adjtrans.jl |
Updates adjoint/transpose mul! wrappers to use hasmethod and new buffers. |
src/cat.jl |
Updates hcat/vcat to construct LinearOperator{T,S} directly (drops args5 propagation). |
src/special-operators.jl |
Updates BlockDiagonalOperator to return LinearOperator{T,S} directly. |
src/shifted_operators.jl |
Makes several fields const and removes use_prod5! method. |
src/lbfgs.jl |
Marks many struct fields const and removes use_prod5! method. |
src/lsr1.jl |
Marks many struct fields const and removes use_prod5! method. |
src/TimedOperators.jl |
Marks wrapped fields const (but still forwards removed API and assumes 4-arg closures). |
src/DiagonalHessianApproximation.jl |
Marks many struct fields const and removes 5-arg bookkeeping fields. |
test/test_linop.jl |
Adjusts expectations around has_args5 for composites by marking several tests as @test_broken. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function TimedLinearOperator(op::AbstractLinearOperator{T}) where {T} | ||
| timer = TimerOutput() | ||
| prod!(res, x, α, β) = @timeit timer "prod" op.prod!(res, x, α, β) | ||
| tprod!(res, x, α, β) = @timeit timer "tprod" op.tprod!(res, x, α, β) | ||
| ctprod!(res, x, α, β) = @timeit timer "ctprod" op.ctprod!(res, x, α, β) | ||
| TimedLinearOperator{T}(timer, op, prod!, tprod!, ctprod!) | ||
| end |
There was a problem hiding this comment.
TimedLinearOperator currently wraps op.prod!/tprod!/ctprod! with 4-arg closures unconditionally. If the wrapped operator was constructed with a 2-arg prod! (supported by LinearOperator(::Type, ..., prod!)), mul! will now take the hasmethod(..., (res,v,α,β)) fast-path and this wrapper will throw a MethodError when it calls op.prod!(res,x,α,β). Consider implementing timed wrappers that support both arities (e.g., define both 2-arg and 4-arg methods, or time mul!(res, op, x, α, β) directly) so timed operators behave the same as the underlying operator.
There was a problem hiding this comment.
This was in old code, but I agree that this was wrong. I fixed it.
| function mul!(res::AbstractMatrix, op::AbstractLinearOperator, m::AbstractMatrix{T}, α, β) where {T} | ||
| op.prod!(res, m, α, β) | ||
| end |
There was a problem hiding this comment.
mul!(res::AbstractMatrix, op::AbstractLinearOperator, m::AbstractMatrix, α, β) always calls op.prod!(res, m, α, β). Operators constructed with a 2-arg prod! (documented in constructors.jl as supported) will throw a MethodError even for the common mul!(res, op, m) case where β == 0. Consider mirroring the vector path: use hasmethod to detect a 4-arg implementation, and otherwise fall back to the 2-arg prod! at least when β is zero (or implement a temp buffer strategy for β ≠ 0).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
==========================================
- Coverage 95.00% 94.86% -0.14%
==========================================
Files 17 21 +4
Lines 1100 1168 +68
==========================================
+ Hits 1045 1108 +63
- Misses 55 60 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@timholy We don't break any dependency with this PR. I am ok to merge it after a last pass on it from you. |
amontoison
merged commit 4de94ea
into
JuliaSmoothOptimizers:main
The
LinearOperatorconstructor was insanely slow (~5μs) because of the call toget_nargs. This removes that call and much of the 5-arg logic, replacing with the use ofhasmethod. Because all the types passed tohasmethodare concrete, Julia's compiler can decide this at compile time, so there is no actual runtime cost to this check.This also removes the internal storage for
has_args5since we can't compute that at construction time without making the constructor very slow. One consequence is a limited test breakage:has_args5now refers to the "surface-level" operator only, and formerly some composite operations (e.g.,hcat,+, etc) would "lie" about whether there was a 5-argmul!available for the composite object in favor of reporting whether that was true for all the constituents. With this change,has_args5will report whether the composite object itself has a 5-argmul!available, which in cases here is "yes." But a few tests were checking that the composite object reported "no".In addition to this change, the fields of
mutable structs that do not seem to be intended to be mutated have been madeconst. This should also result in some performance improvements.Fixes #275
Closes #282