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

Concrete optimizer type in CachingOptimizer #2520

Closed
wants to merge 2 commits into from
Closed

Conversation

blegat
Copy link
Member

@blegat blegat commented Mar 9, 2021

Here are benchmarks (the timing vary quite a lot depending on the % of GC time) so it's more interesting to look at allocations:

using JuMP
function _speed(model, n)
    @time x = [@variable(model) for i in 1:n]
    @time for i in 1:n
        set_lower_bound(x[i], 0.0)
    end
end
function speed1(n)
    model = Model()
    _speed(model, n)
end
function speed2(n)
    model = Model(() -> MOIU.MockOptimizer(MOI.Utilities.Model{Float64}()))
    MOIU.attach_optimizer(model)
    _speed(model, n)
end
function speed3(n)
    model = Model(() -> MOIU.MockOptimizer(MOI.Utilities.Model{Float64}()))
    _speed(model, n)
end
speed1(100000)
speed2(100000)
speed3(100000)

Before:

julia> include("test/perf/many_calls.jl")
  0.016072 seconds (200.05 k allocations: 16.460 MiB, 45.11% gc time)
  0.014611 seconds (300.00 k allocations: 6.104 MiB)
  0.033083 seconds (300.17 k allocations: 29.605 MiB, 19.79% gc time)
  0.059006 seconds (600.07 k allocations: 22.019 MiB)
  0.014988 seconds (200.05 k allocations: 16.460 MiB, 40.04% gc time)
  0.014893 seconds (300.00 k allocations: 6.104 MiB)

After:

julia> include("test/perf/many_calls.jl")
  0.008772 seconds (200.05 k allocations: 16.460 MiB)
  0.015277 seconds (300.00 k allocations: 6.104 MiB)
  0.023118 seconds (200.17 k allocations: 28.080 MiB)
  0.061183 seconds (200.07 k allocations: 15.916 MiB)
  0.008718 seconds (200.05 k allocations: 16.460 MiB)
  0.013356 seconds (200.00 k allocations: 4.578 MiB)

With

function _speed(model, n)
    @time x = [@variable(model, lower_bound=0.0, upper_bound=1.0, integer=true) for i in 1:n]
    @time for i in 1:n
        fix(x[i], 0.5, force=true)
    end
end

Before:

  0.029207 seconds (400.05 k allocations: 19.511 MiB)
  0.041292 seconds (600.00 k allocations: 10.681 MiB)
  0.222799 seconds (1.30 M allocations: 78.879 MiB, 19.79% gc time)
  0.216714 seconds (1.30 M allocations: 32.701 MiB)
  0.033890 seconds (400.05 k allocations: 19.511 MiB)
  0.047814 seconds (600.00 k allocations: 10.681 MiB)

After:

  0.029957 seconds (400.05 k allocations: 19.511 MiB)
  0.044707 seconds (600.00 k allocations: 10.681 MiB)
  0.152942 seconds (200.38 k allocations: 62.094 MiB, 6.97% gc time)
  0.207885 seconds (800.07 k allocations: 25.071 MiB)
  0.030803 seconds (200.05 k allocations: 16.460 MiB)
  0.053592 seconds (600.00 k allocations: 10.681 MiB)

The big allocation drop here from 1.3 M to 200 k is due to https://github.com/jump-dev/JuMP.jl/blob/master/src/variables.jl#L882-L908 where we use the backend like it's type stable but before this PR, the caching optimizer is MOIU.CachingOptimizer{MOI.AbstractOptimizer} and after this PR, it is MOI.CachingOptimizer{MOIU.MockOptimizer{MOIU.Model{Float64}}}.

As the backend type is not concrete, it seems wasteful to also have a non-concrete optimizer type in the optimizer field of the backend.
We can benefit from the non-concretess of the backend in set_optimizer by changing its type.
Another option would be to parametrize the JuMP.Model type by the backend type and have a non-concrete optimizer type for the optimizer of the caching optimizer.
We need at least one to be an abstract type so that we can implement set_optimizer but it seems wasteful to have both of them.

Note that with this PR, we have less allocation once we set the optimizer even if it's empty as the non-concreteness of the optimizer gives allocations even if it is not used it seems.

@odow
Copy link
Member

odow commented Mar 9, 2021

Can we try this on some bigger workloads as well? jump-dev/MathOptInterface.jl#1252 (comment)

@blegat blegat force-pushed the bl/set_optimizer branch from ce5b95d to 6f967cf Compare March 9, 2021 09:36
@blegat
Copy link
Member Author

blegat commented Mar 9, 2021

The other allocations of fix should be fixed by jump-dev/MathOptInterface.jl#1256, #2521 and jump-dev/MathOptInterface.jl#1276

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #2520 (4cfc4f3) into master (a9890a4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2520      +/-   ##
==========================================
- Coverage   93.54%   93.54%   -0.01%     
==========================================
  Files          44       44              
  Lines        5456     5455       -1     
==========================================
- Hits         5104     5103       -1     
  Misses        352      352              
Impacted Files Coverage Δ
src/optimizer_interface.jl 75.51% <100.00%> (+0.51%) ⬆️
src/JuMP.jl 83.82% <0.00%> (-0.14%) ⬇️

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 a9890a4...4cfc4f3. Read the comment docs.

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

Successfully merging this pull request may close these issues.

2 participants