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.

Sign up for GitHub

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

Jump to bottom

Add unsafe_convert method for CbcProblem. Resolve #100. #101

Merged
merged 5 commits into from
Jan 31, 2019
Merged

Add unsafe_convert method for CbcProblem. Resolve #100. #101

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe332f5
Use GC.@preserve where appropriate. Resolve #100.
tkoolen Jan 31, 2019
c59c0db
Ease off the preserves a little.
tkoolen Jan 31, 2019
1aa92c1
unsafe_convert method.
tkoolen Jan 31, 2019
c6b221b
Remove more superfluous preserves.
tkoolen Jan 31, 2019
1538144
Add comment on `unsafe_convert`.
mlubin Jan 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 75 additions & 30 deletions src/CbcCInterface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ function deleteModel(prob::CbcModel)
if prob.p == C_NULL
return
end
@cbc_ccall deleteModel Cvoid (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
@cbc_ccall deleteModel Cvoid (Ptr{Cvoid},) prob.p
end
prob.p = C_NULL
return
end
Expand All @@ -98,7 +100,9 @@ end
macro getproperty(T, name)
@eval function ($name)(prob::CbcModel)
check_problem(prob)
@cbc_ccall $name $T (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
@cbc_ccall $name $T (Ptr{Cvoid},) prob.p
end
end
end

Expand Down Expand Up @@ -136,32 +140,42 @@ function loadProblem(prob::CbcModel,
mat = convert(SparseMatrixCSC{Float64,Int32},constraint_matrix)
nrow,ncol = size(mat)

@cbc_ccall loadProblem Cvoid (Ptr{Cvoid}, Int32, Int32, Ptr{CoinBigIndex},
Ptr{Int32}, Ptr{Float64}, Ptr{Float64}, Ptr{Float64},
Ptr{Float64}, Ptr{Float64}, Ptr{Float64}) prob.p ncol nrow mat.colptr.-Int32(1) mat.rowval.-Int32(1) mat.nzval vec_or_null(Float64, col_lb, ncol) vec_or_null(Float64, col_ub, ncol) vec_or_null(Float64, obj, ncol) vec_or_null(Float64, row_lb, nrow) vec_or_null(Float64, row_ub, nrow)
GC.@preserve prob mat begin
tkoolen marked this conversation as resolved.
Show resolved Hide resolved
@cbc_ccall loadProblem Cvoid (Ptr{Cvoid}, Int32, Int32, Ptr{CoinBigIndex},
Ptr{Int32}, Ptr{Float64}, Ptr{Float64}, Ptr{Float64},
Ptr{Float64}, Ptr{Float64}, Ptr{Float64}) prob.p ncol nrow mat.colptr.-Int32(1) mat.rowval.-Int32(1) mat.nzval vec_or_null(Float64, col_lb, ncol) vec_or_null(Float64, col_ub, ncol) vec_or_null(Float64, obj, ncol) vec_or_null(Float64, row_lb, nrow) vec_or_null(Float64, row_ub, nrow)
end
end

function readMps(prob::CbcModel, filename::String)
check_problem(prob)
@assert isascii(filename)
@cbc_ccall readMps Cint (Ptr{Cvoid}, Ptr{UInt8}) prob.p filename
GC.@preserve prob filename begin
@cbc_ccall readMps Cint (Ptr{Cvoid}, Ptr{UInt8}) prob.p filename
end
end

function writeMps(prob::CbcModel, filename::String)
check_problem(prob)
@assert isascii(filename)
@cbc_ccall writeMps Cint (Ptr{Cvoid}, Ptr{UInt8}) prob.p filename
GC.@preserve prob filename begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filename is a direct argument, no preserve needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this one (and the one you commented on below) I'm not sure of. I think it could currently still be an issue if writeMps happens to get inlined, and given Stefan's comment, there don't seem to be future guarantees regarding input arguments anyway. Again, I'm not sure of the cost of GC.@preserve, but I'd assume it'd be negligible compared to the ccall.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation makes a strong statement about ccall arguments: https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#Garbage-Collection-Safety-1

ccall automatically arranges that all of its arguments will be preserved from garbage collection until the call returns.

I interpret that to mean that if this behavior changes it can only do so In a new major Julia release. I agree that the extra preserve is unlikely to have a performance impact. I'm nit picking so that we apply consistent rules and don't confuse someone who looks at this code and wonders why we preserved this particular argument when it's not needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be cleaner to define a convert(::Type{Ptr{Cvoid}},::CbcModel) method and just pass prob to ccall instead? See https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#Auto-conversion:-1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fully onboard with the nitpicking here since this has ramifications for many other packages. I think @andreasnoack's idea is a good one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, according to https://docs.julialang.org/en/v1/base/c/#Base.cconvert,

Neither convert nor cconvert should take a Julia object and turn it into a Ptr

So I think this should be a Base.unsafe_convert method.

@cbc_ccall writeMps Cint (Ptr{Cvoid}, Ptr{UInt8}) prob.p filename
end
end

function setInitialSolution(prob::CbcModel, array::Vector{Float64})
check_problem(prob)
@cbc_ccall setInitialSolution Cvoid (Ptr{Cvoid}, Ptr{Float64}) prob.p array
GC.@preserve prob array begin
tkoolen marked this conversation as resolved.
Show resolved Hide resolved
@cbc_ccall setInitialSolution Cvoid (Ptr{Cvoid}, Ptr{Float64}) prob.p array
end
end

function problemName(prob::CbcModel)
check_problem(prob)
a = Array(UInt8, 100)
@cbc_ccall problemName Cvoid (Ptr{Cvoid},Cint,Ptr{UInt8}) prob.p 100 a
GC.@preserve prob begin
@cbc_ccall problemName Cvoid (Ptr{Cvoid},Cint,Ptr{UInt8}) prob.p 100 a
end
return string(a)
end

Expand All @@ -172,26 +186,34 @@ end

function getNumElements(prob::CbcModel)
check_problem(prob)
@cbc_ccall getNumElements Cint (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
@cbc_ccall getNumElements Cint (Ptr{Cvoid},) prob.p
end
end

function getVectorStarts(prob::CbcModel)
check_problem(prob)
p = @cbc_ccall getVectorStarts Ptr{CoinBigIndex} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall getVectorStarts Ptr{CoinBigIndex} (Ptr{Cvoid},) prob.p
end
num_cols = Int(getNumCols(prob))
return copy(unsafe_wrap(Array,p,(num_cols+1,)))
end

function getIndices(prob::CbcModel)
check_problem(prob)
p = @cbc_ccall getIndices Ptr{Cint} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall getIndices Ptr{Cint} (Ptr{Cvoid},) prob.p
end
nnz = Int(getNumElements(prob))
return copy(unsafe_wrap(Array,p,(nnz,)))
end

function getElements(prob::CbcModel)
check_problem(prob)
p = @cbc_ccall getElements Ptr{Float64} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall getElements Ptr{Float64} (Ptr{Cvoid},) prob.p
end
nnz = Int(getNumElements(prob))
return copy(unsafe_wrap(Array,p,(nnz,)))
end
Expand All @@ -205,7 +227,9 @@ end
# 1 : minimize, -1 : maximize
function setObjSense(prob::CbcModel, sense)
check_problem(prob)
@cbc_ccall setObjSense Cvoid (Ptr{Cvoid}, Float64) prob.p sense
GC.@preserve prob begin
@cbc_ccall setObjSense Cvoid (Ptr{Cvoid}, Float64) prob.p sense
end
end

@getproperty Float64 getObjSense
Expand All @@ -214,7 +238,9 @@ for s in (:getRowLower, :getRowUpper, :getRowActivity)
@eval function ($s)(prob::CbcModel)
check_problem(prob)
nrow = Int(getNumRows(prob))
p = @cbc_ccall $s Ptr{Float64} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall $s Ptr{Float64} (Ptr{Cvoid},) prob.p
end
return copy(unsafe_wrap(Array,p,(nrow,)))
end
end
Expand All @@ -223,36 +249,48 @@ for s in (:getColLower, :getColUpper, :getObjCoefficients, :getColSolution)
@eval function ($s)(prob::CbcModel)
check_problem(prob)
ncol = Int(getNumCols(prob))
p = @cbc_ccall $s Ptr{Float64} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall $s Ptr{Float64} (Ptr{Cvoid},) prob.p
end
return copy(unsafe_wrap(Array,p,(ncol,)))
end
end

for s in (:setRowUpper, :setRowLower, :setObjCoeff, :setColLower, :setColUpper)
@eval function($s)(prob::CbcModel, index::Integer, value::Float64)
check_problem(prob)
@cbc_ccall $s Cvoid (Ptr{Cvoid}, Cint, Float64) prob.p index value
GC.@preserve prob begin
@cbc_ccall $s Cvoid (Ptr{Cvoid}, Cint, Float64) prob.p index value
end
end
end

function isInteger(prob::CbcModel, index::Integer)
check_problem(prob)
v = @cbc_ccall isInteger Cint (Ptr{Cvoid},Cint) prob.p index
GC.@preserve prob index begin
v = @cbc_ccall isInteger Cint (Ptr{Cvoid},Cint) prob.p index
end
return v == 1
end

function setContinuous(prob::CbcModel, index::Integer)
check_problem(prob)
@cbc_ccall setContinuous Cvoid (Ptr{Cvoid},Cint) prob.p index
GC.@preserve prob index begin
@cbc_ccall setContinuous Cvoid (Ptr{Cvoid},Cint) prob.p index
end
end

function setInteger(prob::CbcModel, index::Integer)
check_problem(prob)
@cbc_ccall setInteger Cvoid (Ptr{Cvoid},Cint) prob.p index
GC.@preserve prob begin
@cbc_ccall setInteger Cvoid (Ptr{Cvoid},Cint) prob.p index
end
end

function Base.copy(prob::CbcModel)
p = @cbc_ccall clone Ptr{Cvoid} (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
p = @cbc_ccall clone Ptr{Cvoid} (Ptr{Cvoid},) prob.p
end
prob = CbcModel(p)
@compat finalizer(deleteModel, prob)
return prob
Expand All @@ -261,13 +299,17 @@ end
function setParameter(prob::CbcModel, name::String, value::String)
@assert isascii(name)
@assert isascii(value)
@cbc_ccall setParameter Cvoid (Ptr{Cvoid},Ptr{UInt8},Ptr{UInt8}) prob.p name value
GC.@preserve prob name begin
@cbc_ccall setParameter Cvoid (Ptr{Cvoid},Ptr{UInt8},Ptr{UInt8}) prob.p name value
end
end

# TODO: registerCallBack clearCallBack

function solve(prob::CbcModel)
@cbc_ccall solve Cint (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
@cbc_ccall solve Cint (Ptr{Cvoid},) prob.p
end
end

@getproperty Float64 sumPrimalInfeasibilities
Expand All @@ -281,7 +323,9 @@ for s in (:isAbandoned, :isProvenOptimal, :isProvenInfeasible, :isContinuousUnbo
:isNodeLimitReached, :isSecondsLimitReached, :isSolutionLimitReached, :isInitialSolveAbandoned, :isInitialSolveProvenOptimal, :isInitialSolveProvenPrimalInfeasible)
@eval function ($s)(prob::CbcModel)
check_problem(prob)
v = @cbc_ccall $s Cint (Ptr{Cvoid},) prob.p
GC.@preserve prob begin
v = @cbc_ccall $s Cint (Ptr{Cvoid},) prob.p
end
return v != 0
end
end
Expand All @@ -295,12 +339,13 @@ end
function addSOS(prob::CbcModel, numRows::Integer, rowStarts::Vector{Cint},
colIndices::Vector{Cint}, weights::Vector{Float64}, typ::Integer)

@cbc_ccall(addSOS,Cvoid,(Ptr{Cvoid}, Cint, Ptr{Cint}, Ptr{Cint},
Ptr{Float64}, Cint),prob.p,numRows,
rowStarts .- convert(Cint,1),
colIndices .- convert(Cint,1),
weights, typ)

GC.@preserve prob colIndices weights begin
@cbc_ccall(addSOS,Cvoid,(Ptr{Cvoid}, Cint, Ptr{Cint}, Ptr{Cint},
Ptr{Float64}, Cint),prob.p,numRows,
rowStarts .- convert(Cint,1),
colIndices .- convert(Cint,1),
weights, typ)
end
end

# see Cbc_C_Interface.h documentation
Expand Down