Improve performance of coefficient modification #135
Conversation
|
Open it against master. Now that there is a tagged version of LQOI, I'll fix the other PR and then merge it. |
|
BTW, this is great that it's getting tidied up. I am in favour of deprecating some of the weird helper methods to get closer to the C. Very few people should be using Gurobi.jl natively. |
rdeits
force-pushed
the
rd/coef-performance
branch
from
8dab2e2 to
ebad4ac
Compare
rdeits
force-pushed
the
rd/coef-performance
branch
from
ebad4ac to
b05e110
Compare
|
Ok, sounds good. I've changed the base branch of the PR and rebased the commits. Tests are passing for me locally with Gurobi 8.0.0 on Ubuntu 18.04 |
| @@ -314,11 +314,23 @@ function del_constrs!(model::Model, idx::Vector{Cint}) | |||
| end | |||
| end | |||
|
|
|||
| function chg_coeffs!(model::Model, cidx::Real, vidx::Real, val::Real) | |||
There was a problem hiding this comment.
Are these types okay? Shouldn't cidx and vidx be Cints and val be a Float64?
There was a problem hiding this comment.
The types specified in the ccall call are Ref{Cint} and Ref{Float64}, so Julia will automatically insert a call to Base.cconvert which will do the correct conversion (or fail with a MethodError). For example:
julia> Base.cconvert(Ref{Cint}, 1)
Base.RefValue{Int32}(1)
julia> Base.cconvert(Ref{Cint}, Int8(1))
Base.RefValue{Int32}(1)
julia> Base.cconvert(Ref{Cint}, 1.0)
Base.RefValue{Int32}(1)There was a problem hiding this comment.
Has it always done this? I'm sure I've been stung by it
There was a problem hiding this comment.
I'm not sure. I think a lot of this code was written before I'd even heard of Julia...
There was a problem hiding this comment.
Yeah a lot of it can be cleaned up
| @@ -346,8 +359,17 @@ function getcoeff!(val::FVec, model::Model, cidx::Integer, vidx::Integer) | |||
| throw(GurobiError(model.env, ret)) | |||
| end | |||
| end | |||
|
|
|||
| function getcoeff(model::Model, cidx::Integer, vidx::Integer) | |||
There was a problem hiding this comment.
I just added a couple more tests to verify that this works fine even when cidx and vidx are not ::Cint.
chg_coeffswhen called with scalar indices was previously allocating a length-1 vector of indices and values, which is pretty inefficient. This PR changes that to using Refs, which are quite a bit cheaper.getcoeffwas also unnecessarily allocating a vector for its return type, so I changed that as well. I deprecated thegetcoeff!method which operated on that unnecessary vector.I also added some unit tests to verify the basics of getting and setting coefficients.
Setting a single coefficient goes from
214.000 ns (5 allocations: 480 bytes)to89.000 ns (3 allocations: 48 bytes)after this PR. Performance should improve even more in v0.7, since the Refs won't need to be heap-allocated.I also fixed some remaining deprecation warnings in the MPB tests.
@odow this is a PR against your branch in #125 . Let me know if I should open it against
masterinstead.