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

Add caching #173

Closed
wants to merge 4 commits into from
Closed

Add caching #173

wants to merge 4 commits into from

Conversation

DrChainsaw
Copy link

@DrChainsaw DrChainsaw commented Aug 17, 2021

This is the proposed fix for #172

Unfortunately, only caching of the solution seems to have a pretty disappointing impact on the benchmark:

julia> function bench(optimizer, N = 30_000)
           model = Model(with_optimizer(optimizer))
           @variable(model, x[i = 1:N] >= i)
           @objective(model, Min, sum(x))
           optimize!(model)
           @benchmark value.($x)
        end
bench (generic function with 2 methods)

julia> bench(Cbc.Optimizer)
Optimal - objective value 4.50015e+08
Optimal objective 450015000 - 0 iterations time 0.002
BenchmarkTools.Trial: 2 samples with 1 evaluation.
 Range (min  max):  3.524 s     4.264 s  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     3.894 s               ┊ GC (median):    0.00%        
 Time  (mean ± σ):   3.894 s ± 523.174 ms  ┊ GC (mean ± σ):  0.00% ± 0.00%

  █                                                        █  
  █▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█ ▁
  3.52 s         Histogram: frequency by time         4.26 s <

Profiling shows that Cbc_isProvenOptimal and Cbc_getNumIntegers seems to dominate the computation time.

After adding caching of those two performance appears to be back on track:

julia> bench(Cbc.Optimizer)
Optimal - objective value 4.50015e+08
Optimal objective 450015000 - 0 iterations time 0.002
BenchmarkTools.Trial: 205 samples with 1 evaluation.
 Range (min  max):  19.858 ms  34.276 ms  ┊ GC (min  max): 0.00%  13.42%
 Time  (median):     23.973 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   24.479 ms ±  2.629 ms  ┊ GC (mean ± σ):  1.76% ±  4.52%

  ▄   █                               ▄  █                    ▁
  █▁▁▁█▁▁▁▁▁▁▁▁▆▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▆▁▁▁█▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ █
  20.4 ms         Histogram: frequency by time        23.4 ms <

 Memory estimate: 6.18 MiB, allocs estimate: 330002.

There are probably alot of other Cbc_X functions which are also too slow, but I lack the overall understanding to pinpoint them.

I have tried to make a minimum of changes for easier reviewing of the "core changes", but please suggest better structure and names if you think this is the way to go. I don't mind nitpicking.

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #173 (b9f4ac8) into master (4a80397) will increase coverage by 0.07%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   53.48%   53.55%   +0.07%     
==========================================
  Files           3        3              
  Lines         531      534       +3     
==========================================
+ Hits          284      286       +2     
- Misses        247      248       +1     
Impacted Files Coverage Δ
src/MOI_wrapper/MOI_wrapper.jl 80.30% <94.11%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a80397...b9f4ac8. Read the comment docs.

@odow
Copy link
Member

odow commented Aug 17, 2021

The fact that it's all the Cbc_ functions makes me think there is a bigger issue going on...

Cache solution_status eagerly
@DrChainsaw
Copy link
Author

The fact that it's all the Cbc_ functions makes me think there is a bigger issue going on...

I agree!

@odow
Copy link
Member

odow commented Aug 17, 2021

What's the benchmark with your latest changes?

@odow
Copy link
Member

odow commented Sep 15, 2021

Just a note to say that I haven't forgotten about this PR. But I've been busy with the MOI 0.10 release so I haven't had a chance to take a deeper look yet.

@DrChainsaw
Copy link
Author

No problem at all!

I'm not blocked by this, just trying to do my duty as a user and report issues (and fix what I feel is within reach for me to fix). Let me know if there is anything more I can do (the "deeper issue" feels a bit out of reach for me atm).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants