Skip to content

Commit a2de5b0

Browse files
committed
Remove BLAS timing and make logging more efficient
Based on review feedback: 1. Removed all BLAS timing functionality - Removed time_blas_operation function - Removed blas_timing verbosity setting - Removed timing from documentation and tests 2. Made get_blas_operation_info conditional on logging - Only calls get_blas_operation_info when logging is actually enabled - Avoids unnecessary computation when logging is disabled 3. Made condition number computation optional - Added compute_condition parameter (default: false) - Only computes condition number when explicitly requested - Avoids expensive computation for large matrices 4. Updated BLISLUFactorization extension - Removed timing calls - Only gathers operation info when needed for logging - More efficient checking of verbosity levels 5. Updated documentation - Removed all references to timing functionality - Documented optional condition number computation 6. Updated tests - Removed timing tests - Added tests for optional condition number computation These changes make the logging more efficient by avoiding unnecessary computations when logging is disabled. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent f998aa1 commit a2de5b0

File tree

5 files changed

+38
-95
lines changed

5 files changed

+38
-95
lines changed

docs/src/basics/common_solver_opts.md

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@ sol = solve(prob; verbose=verbose)
111111
- blas_invalid_args: Controls messages when BLAS/LAPACK receives invalid arguments (default: Error)
112112
##### Performance Settings
113113
- no_right_preconditioning: Controls messages when right preconditioning is not used (default: Warn)
114-
- blas_timing: Controls performance timing messages for BLAS/LAPACK operations (default: None)
115114
##### Numerical Settings
116115
- using_IterativeSolvers: Controls messages when using the IterativeSolvers.jl package (default: Warn)
117116
- IterativeSolvers_iterations: Controls messages about iteration counts from IterativeSolvers.jl (default: Warn)
@@ -145,7 +144,6 @@ using LinearSolve
145144
# Enable detailed BLAS error logging
146145
verbose = LinearVerbosity(
147146
blas_errors = Verbosity.Info(), # Show detailed error interpretations
148-
blas_timing = Verbosity.Info(), # Show performance metrics
149147
blas_info = Verbosity.Info() # Show all BLAS operation details
150148
)
151149

@@ -163,7 +161,7 @@ sol = solve(prob, LUFactorization(); verbose=verbose)
163161
```
164162

165163
The enhanced logging also provides:
166-
- Matrix properties (size, type, condition number when feasible)
164+
- Matrix properties (size, type, element type)
167165
- Memory usage estimates
168-
- Performance timing for BLAS operations (when blas_timing is enabled)
169-
- Detailed context for debugging numerical issues
166+
- Detailed context for debugging numerical issues
167+
- Optional condition number computation (can be enabled via compute_condition parameter)

ext/LinearSolveBLISExt.jl

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ using LinearAlgebra.LAPACK: require_one_based_indexing, chkfinite, chkstride1,
1111
@blasfunc, chkargsok
1212
using LinearSolve: ArrayInterface, BLISLUFactorization, @get_cacheval, LinearCache, SciMLBase,
1313
interpret_blas_code, log_blas_info, get_blas_operation_info,
14-
time_blas_operation, check_and_log_lapack_result, LinearVerbosity
14+
check_and_log_lapack_result, LinearVerbosity
1515
using SciMLBase: ReturnCode
1616
using SciMLLogging: Verbosity
1717

@@ -228,22 +228,23 @@ function SciMLBase.solve!(cache::LinearCache, alg::BLISLUFactorization;
228228
if cache.isfresh
229229
cacheval = @get_cacheval(cache, :BLISLUFactorization)
230230

231-
# Get additional operation info for logging
232-
op_info = get_blas_operation_info(:dgetrf, A, cache.b)
233-
234-
# Time the BLAS operation if verbosity requires it
235-
res = time_blas_operation(:dgetrf, verbose) do
236-
getrf!(A; ipiv = cacheval[1].ipiv, info = cacheval[2])
237-
end
231+
# Perform the factorization
232+
res = getrf!(A; ipiv = cacheval[1].ipiv, info = cacheval[2])
238233

239234
fact = LU(res[1:3]...), res[4]
240235
cache.cacheval = fact
241236

242-
# Log BLAS return code with detailed interpretation
237+
# Log BLAS return code with detailed interpretation if logging is enabled
243238
info_value = res[3]
244239
if info_value != 0
245-
log_blas_info(:dgetrf, info_value, verbose; extra_context=op_info)
246-
elseif isa(verbose.numerical.blas_success, Verbosity.Info)
240+
# Only get operation info if we need to log
241+
if !(verbose.numerical.blas_errors isa Verbosity.None)
242+
op_info = get_blas_operation_info(:dgetrf, A, cache.b)
243+
log_blas_info(:dgetrf, info_value, verbose; extra_context=op_info)
244+
end
245+
elseif !(verbose.numerical.blas_success isa Verbosity.None)
246+
# Only get operation info if we need to log success
247+
op_info = get_blas_operation_info(:dgetrf, A, cache.b)
247248
@info "BLAS LU factorization (dgetrf) completed successfully" op_info
248249
end
249250

@@ -258,20 +259,18 @@ function SciMLBase.solve!(cache::LinearCache, alg::BLISLUFactorization;
258259
require_one_based_indexing(cache.u, cache.b)
259260
m, n = size(A, 1), size(A, 2)
260261

261-
# Time the solve operation
262-
solve_result = time_blas_operation(:dgetrs, verbose) do
263-
if m > n
264-
Bc = copy(cache.b)
265-
getrs!('N', A.factors, A.ipiv, Bc; info)
266-
copyto!(cache.u, 1, Bc, 1, n)
267-
else
268-
copyto!(cache.u, cache.b)
269-
getrs!('N', A.factors, A.ipiv, cache.u; info)
270-
end
262+
# Perform the solve
263+
if m > n
264+
Bc = copy(cache.b)
265+
getrs!('N', A.factors, A.ipiv, Bc; info)
266+
copyto!(cache.u, 1, Bc, 1, n)
267+
else
268+
copyto!(cache.u, cache.b)
269+
getrs!('N', A.factors, A.ipiv, cache.u; info)
271270
end
272271

273272
# Log solve operation result if there was an error
274-
if info[] != 0
273+
if info[] != 0 && !(verbose.numerical.blas_errors isa Verbosity.None)
275274
log_blas_info(:dgetrs, info[], verbose)
276275
end
277276

src/blas_logging.jl

Lines changed: 5 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -190,20 +190,21 @@ end
190190

191191
# Extended information for specific BLAS operations
192192
"""
193-
get_blas_operation_info(func::Symbol, A, b=nothing)
193+
get_blas_operation_info(func::Symbol, A, b=nothing; compute_condition=false)
194194
195195
Get additional information about a BLAS operation for enhanced logging.
196+
Set compute_condition=true to include condition number computation (may be expensive).
196197
"""
197-
function get_blas_operation_info(func::Symbol, A, b=nothing)
198+
function get_blas_operation_info(func::Symbol, A, b=nothing; compute_condition=false)
198199
info = Dict{Symbol,Any}()
199200

200201
# Matrix properties
201202
info[:matrix_size] = size(A)
202203
info[:matrix_type] = typeof(A)
203204
info[:element_type] = eltype(A)
204205

205-
# Condition number (if not too expensive)
206-
if size(A, 1) <= 1000 && size(A, 1) == size(A, 2)
206+
# Condition number (only if explicitly requested)
207+
if compute_condition && size(A, 1) == size(A, 2)
207208
try
208209
info[:condition_number] = cond(A)
209210
catch
@@ -224,44 +225,3 @@ function get_blas_operation_info(func::Symbol, A, b=nothing)
224225
return info
225226
end
226227

227-
# Performance timing for BLAS operations
228-
mutable struct BLASTimingInfo
229-
operation::Symbol
230-
start_time::Float64
231-
end_time::Float64
232-
allocated_bytes::Int
233-
gc_time::Float64
234-
end
235-
236-
"""
237-
time_blas_operation(f::Function, func::Symbol, verbose::LinearVerbosity)
238-
239-
Time a BLAS operation and log performance information based on verbosity.
240-
"""
241-
function time_blas_operation(f::Function, func::Symbol, verbose::LinearVerbosity)
242-
if verbose.performance.blas_timing isa Verbosity.None
243-
return f()
244-
end
245-
246-
# Measure performance
247-
stats = @timed f()
248-
result = stats.value
249-
250-
# Log timing information
251-
if verbose.performance.blas_timing isa Verbosity.Info ||
252-
verbose.performance.blas_timing isa Verbosity.Warn
253-
timing_msg = format_timing_message(func, stats)
254-
log_with_verbosity(verbose.performance.blas_timing, timing_msg, :performance)
255-
end
256-
257-
return result
258-
end
259-
260-
function format_timing_message(func::Symbol, stats)
261-
parts = String[]
262-
push!(parts, "BLAS/LAPACK $func performance:")
263-
push!(parts, " Time: $(round(stats.time, sigdigits=3)) seconds")
264-
push!(parts, " Allocations: $(round(stats.bytes / 1024^2, digits=2)) MB")
265-
push!(parts, " GC time: $(round(stats.gctime, sigdigits=3)) seconds")
266-
return join(parts, "\n")
267-
end

src/verbosity.jl

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,7 @@ const linear_defaults = Dict{Symbol, Verbosity.Type}(
1313
:blas_errors => Verbosity.Warn(),
1414
:blas_info => Verbosity.None(),
1515
:blas_success => Verbosity.None(),
16-
:blas_invalid_args => Verbosity.Error(),
17-
:blas_timing => Verbosity.None()
16+
:blas_invalid_args => Verbosity.Error()
1817
)
1918
mutable struct LinearErrorControlVerbosity
2019
default_lu_fallback::Verbosity.Type
@@ -51,12 +50,10 @@ end
5150

5251
mutable struct LinearPerformanceVerbosity
5352
no_right_preconditioning::Verbosity.Type
54-
blas_timing::Verbosity.Type
5553

5654
function LinearPerformanceVerbosity(;
57-
no_right_preconditioning = linear_defaults[:no_right_preconditioning],
58-
blas_timing = linear_defaults[:blas_timing])
59-
new(no_right_preconditioning, blas_timing)
55+
no_right_preconditioning = linear_defaults[:no_right_preconditioning])
56+
new(no_right_preconditioning)
6057
end
6158

6259
function LinearPerformanceVerbosity(verbose::Verbosity.Type)

test/test_blas_logging.jl

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,19 @@ using SciMLLogging: Verbosity
3333
end
3434

3535
@testset "BLAS Operation Info" begin
36-
# Test getting operation info
36+
# Test getting operation info without condition number
3737
A = rand(10, 10)
3838
b = rand(10)
3939
info = LinearSolve.get_blas_operation_info(:dgetrf, A, b)
4040

4141
@test info[:matrix_size] == (10, 10)
4242
@test info[:element_type] == Float64
43-
@test haskey(info, :condition_number)
43+
@test !haskey(info, :condition_number) # Should not compute by default
4444
@test info[:memory_usage_MB] >= 0 # Memory can be 0 for very small matrices
45+
46+
# Test with condition number computation enabled
47+
info_with_cond = LinearSolve.get_blas_operation_info(:dgetrf, A, b; compute_condition=true)
48+
@test haskey(info_with_cond, :condition_number)
4549
end
4650

4751
@testset "Verbosity Integration" begin
@@ -64,7 +68,6 @@ using SciMLLogging: Verbosity
6468
# Test with all logging enabled
6569
verbose_all = LinearVerbosity(
6670
blas_errors = Verbosity.Info(),
67-
blas_timing = Verbosity.Info(),
6871
blas_info = Verbosity.Info()
6972
)
7073

@@ -98,17 +101,3 @@ using SciMLLogging: Verbosity
98101
end
99102
end
100103

101-
@testset "BLAS Timing" begin
102-
# Simple test of timing functionality
103-
A = rand(100, 100)
104-
b = rand(100)
105-
106-
verbose = LinearVerbosity(
107-
blas_timing = Verbosity.Info()
108-
)
109-
110-
# Test that timing doesn't break the solve
111-
prob = LinearProblem(A, b)
112-
sol = solve(prob, LUFactorization(); verbose=verbose)
113-
@test sol.retcode == ReturnCode.Success
114-
end

0 commit comments

Comments
 (0)