From c246e25d0135bb882ea2e2e140f706b991b6aefd Mon Sep 17 00:00:00 2001 From: Joaquim Garcia Date: Sat, 25 Jul 2020 21:17:16 -0300 Subject: [PATCH 1/5] Improve dense dict stability and indexmap copying --- src/Utilities/cachingoptimizer.jl | 55 +++++++++++++++++++++++++++---- src/Utilities/copy.jl | 5 +-- src/Utilities/dense_dict.jl | 10 ++++-- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/Utilities/cachingoptimizer.jl b/src/Utilities/cachingoptimizer.jl index 4401b16f8a..d05628c5ab 100644 --- a/src/Utilities/cachingoptimizer.jl +++ b/src/Utilities/cachingoptimizer.jl @@ -148,16 +148,57 @@ function attach_optimizer(model::CachingOptimizer) # We do not need to copy names because name-related operations are handled by `m.model_cache` indexmap = MOI.copy_to(model.optimizer, model.model_cache, copy_names=false) model.state = ATTACHED_OPTIMIZER - # MOI does not define the type of index_map, so we have to copy it into a - # concrete container. Also load the reverse map. - model.model_to_optimizer_map = IndexMap() - model.optimizer_to_model_map = IndexMap() - for k in keys(indexmap) - model.model_to_optimizer_map[k] = indexmap[k] - model.optimizer_to_model_map[indexmap[k]] = k + # MOI does not define the type of index_map, so we might have to copy it + # into a concrete container. Also load the reverse map. + model.model_to_optimizer_map = convert(IndexMap, indexmap)#IndexMap() + model.optimizer_to_model_map = reverse_index_map(indexmap)#IndexMap() + nothing +end + +function reverse_index_map(src::IndexMap) + dest = IndexMap() + sizehint!(dest.varmap, length(src.varmap)) + # sizehint!(dest.conmap, length(src.conmap)) + reverse_dict(dest.varmap, src.varmap) + reverse_dict(dest.conmap, src.conmap) + return dest +end +function reverse_dict(dest::AbstractDict, src::AbstractDict) + for (k,v) in src + dest[v] = k end end +function Base.convert(::Type{IndexMap}, d::AbstractDict{MOI.Index, MOI.Index}) + map = IndexMap() + for (k,v) in d + map[k] = v + end + return map +end +function Base.convert(::Type{IndexMap}, d::IndexMap) + # return d + # if we return d as is, its not possible to add variables + # if there was a dense dict inside + # the solution would be to allow automatically swtiching + # a ClevelDenseDict... + return IndexMap(standard_dict(d.varmap), d.conmap) +end +function standard_dict(d::D)::D where + {D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex}} + return d +end +function standard_dict(d::D + )::Dict{MOI.VariableIndex, MOI.VariableIndex} where + {D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex}} + ret = Dict{MOI.VariableIndex, MOI.VariableIndex}() + sizehint!(ret, length(d)) + for (k,v) in d + ret[k] = v + end + return d +end + function MOI.copy_to(m::CachingOptimizer, src::MOI.ModelLike; kws...) return automatic_copy_to(m, src; kws...) end diff --git a/src/Utilities/copy.jl b/src/Utilities/copy.jl index 666a47039e..2a24dd0a8a 100644 --- a/src/Utilities/copy.jl +++ b/src/Utilities/copy.jl @@ -63,9 +63,10 @@ error in case `copy_to` is called with `copy_names` equal to `true`. """ supports_default_copy_to(model::MOI.ModelLike, copy_names::Bool) = false -const DenseVariableDict{V} = DenseDict{MOI.VariableIndex, V, typeof(MOI.index_value), typeof(MOI.VariableIndex)} +_index_to_variable(i) = MOI.VariableIndex(i) +const DenseVariableDict{V} = DenseDict{MOI.VariableIndex, V, typeof(MOI.index_value), typeof(_index_to_variable)} function dense_variable_dict(::Type{V}, n) where V - return DenseDict{MOI.VariableIndex, V}(MOI.index_value, MOI.VariableIndex, n) + return DenseDict{MOI.VariableIndex, V}(MOI.index_value, _index_to_variable, n) end struct IndexMap <: AbstractDict{MOI.Index, MOI.Index} diff --git a/src/Utilities/dense_dict.jl b/src/Utilities/dense_dict.jl index a7ac300baf..4c31536a48 100644 --- a/src/Utilities/dense_dict.jl +++ b/src/Utilities/dense_dict.jl @@ -22,15 +22,21 @@ end # Implementation of the `AbstractDict` API. # Base.empty(::DenseDict, ::Type{K}, ::Type{V}) not implemented -function Base.iterate(d::DenseDict, args...) +function Base.iterate(d::DenseDict{K,V}, args...) where {K,V} itr = iterate(d.set, args...) if itr === nothing return nothing else el, i = itr - return d.inverse_hash(el) => d.map[el], i + return inverse_hash(d, el)::K => d.map[el]::V, i end end + +function inverse_hash(d::DenseDict{K, V, F, I}, el)::K where {K,V,F,I} + f = d.inverse_hash::I + return f(el)::K +end + Base.length(d::DenseDict) = length(d.set) Base.haskey(dict::DenseDict, key) = dict.hash(key) in dict.set Base.getindex(dict::DenseDict, key) = dict.map[dict.hash(key)] From 5df41709b3ddb45dc360394901c8dabb97f68f7c Mon Sep 17 00:00:00 2001 From: Joaquim Garcia Date: Sun, 26 Jul 2020 01:18:15 -0300 Subject: [PATCH 2/5] formatting and comments --- src/Utilities/cachingoptimizer.jl | 42 +++++++++++++++++++------------ src/Utilities/copy.jl | 8 ++++++ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/Utilities/cachingoptimizer.jl b/src/Utilities/cachingoptimizer.jl index d05628c5ab..448b9e7fba 100644 --- a/src/Utilities/cachingoptimizer.jl +++ b/src/Utilities/cachingoptimizer.jl @@ -145,25 +145,32 @@ errors can be thrown. """ function attach_optimizer(model::CachingOptimizer) @assert model.state == EMPTY_OPTIMIZER - # We do not need to copy names because name-related operations are handled by `m.model_cache` + # We do not need to copy names because name-related operations are handled + # by `m.model_cache` indexmap = MOI.copy_to(model.optimizer, model.model_cache, copy_names=false) model.state = ATTACHED_OPTIMIZER - # MOI does not define the type of index_map, so we might have to copy it - # into a concrete container. Also load the reverse map. - model.model_to_optimizer_map = convert(IndexMap, indexmap)#IndexMap() - model.optimizer_to_model_map = reverse_index_map(indexmap)#IndexMap() - nothing + # MOI does not define the type of index_map, so we have to convert it + # into an actual IndexMap. Also load the reverse IndexMap. + model.model_to_optimizer_map = convert(IndexMap, indexmap) + model.optimizer_to_model_map = reverse_index_map(indexmap) + return nothing end function reverse_index_map(src::IndexMap) dest = IndexMap() sizehint!(dest.varmap, length(src.varmap)) - # sizehint!(dest.conmap, length(src.conmap)) - reverse_dict(dest.varmap, src.varmap) - reverse_dict(dest.conmap, src.conmap) + _reverse_dict(dest.varmap, src.varmap) + _reverse_dict(dest.conmap, src.conmap) return dest end -function reverse_dict(dest::AbstractDict, src::AbstractDict) + +""" + _reverse_dict(dest::AbstractDict, src::AbstractDict) + +Reverse dictionary so that values of `src` are key of `dest` and vice-versa. +`dest` must be empty. Also the values of `src` are assumed to be unique. +""" +function _reverse_dict(dest::AbstractDict, src::AbstractDict) for (k,v) in src dest[v] = k end @@ -182,15 +189,18 @@ function Base.convert(::Type{IndexMap}, d::IndexMap) # if there was a dense dict inside # the solution would be to allow automatically swtiching # a ClevelDenseDict... - return IndexMap(standard_dict(d.varmap), d.conmap) + return IndexMap(_standard_dict(d.varmap), d.conmap) end -function standard_dict(d::D)::D where - {D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex}} +function _standard_dict( + d::D +)::D where {D<:Dict{MOI.VariableIndex, MOI.VariableIndex}} return d end -function standard_dict(d::D - )::Dict{MOI.VariableIndex, MOI.VariableIndex} where - {D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex}} +function _standard_dict( + d::D +)::Dict{MOI.VariableIndex, MOI.VariableIndex} where { + D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex} +} ret = Dict{MOI.VariableIndex, MOI.VariableIndex}() sizehint!(ret, length(d)) for (k,v) in d diff --git a/src/Utilities/copy.jl b/src/Utilities/copy.jl index 2a24dd0a8a..ee7bc7485b 100644 --- a/src/Utilities/copy.jl +++ b/src/Utilities/copy.jl @@ -63,6 +63,14 @@ error in case `copy_to` is called with `copy_names` equal to `true`. """ supports_default_copy_to(model::MOI.ModelLike, copy_names::Bool) = false +""" + _index_to_variable(i::Int) + +Simply returns `MOI.VariableIndex(i)`. This is necessary to pass a function that +creates the `VariableIndex` from an integer. If we pass `MOI.VariableIndex` +julia understands is as a `DataType` and not a function, this leads to type +instability issues. +""" _index_to_variable(i) = MOI.VariableIndex(i) const DenseVariableDict{V} = DenseDict{MOI.VariableIndex, V, typeof(MOI.index_value), typeof(_index_to_variable)} function dense_variable_dict(::Type{V}, n) where V From af6ebcb0a0b490872bc2323252a159f550f5b437 Mon Sep 17 00:00:00 2001 From: Joaquim Garcia Date: Sun, 26 Jul 2020 14:02:12 -0300 Subject: [PATCH 3/5] name fixes --- src/Utilities/cachingoptimizer.jl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Utilities/cachingoptimizer.jl b/src/Utilities/cachingoptimizer.jl index 448b9e7fba..cc9124803e 100644 --- a/src/Utilities/cachingoptimizer.jl +++ b/src/Utilities/cachingoptimizer.jl @@ -151,12 +151,12 @@ function attach_optimizer(model::CachingOptimizer) model.state = ATTACHED_OPTIMIZER # MOI does not define the type of index_map, so we have to convert it # into an actual IndexMap. Also load the reverse IndexMap. - model.model_to_optimizer_map = convert(IndexMap, indexmap) - model.optimizer_to_model_map = reverse_index_map(indexmap) + model.model_to_optimizer_map = _standardize(indexmap) + model.optimizer_to_model_map = _reverse_index_map(indexmap) return nothing end -function reverse_index_map(src::IndexMap) +function _reverse_index_map(src::IndexMap) dest = IndexMap() sizehint!(dest.varmap, length(src.varmap)) _reverse_dict(dest.varmap, src.varmap) @@ -176,14 +176,14 @@ function _reverse_dict(dest::AbstractDict, src::AbstractDict) end end -function Base.convert(::Type{IndexMap}, d::AbstractDict{MOI.Index, MOI.Index}) +function _standardize(d::AbstractDict{MOI.Index, MOI.Index}) map = IndexMap() for (k,v) in d map[k] = v end return map end -function Base.convert(::Type{IndexMap}, d::IndexMap) +function _standardize(d::IndexMap) # return d # if we return d as is, its not possible to add variables # if there was a dense dict inside From df59fc53341ab87786c982ed2bd74fb363b12250 Mon Sep 17 00:00:00 2001 From: Joaquim Garcia Date: Sun, 26 Jul 2020 19:16:56 -0300 Subject: [PATCH 4/5] simplify dense dict --- src/Utilities/dense_dict.jl | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Utilities/dense_dict.jl b/src/Utilities/dense_dict.jl index 4c31536a48..c38d3489b3 100644 --- a/src/Utilities/dense_dict.jl +++ b/src/Utilities/dense_dict.jl @@ -28,15 +28,10 @@ function Base.iterate(d::DenseDict{K,V}, args...) where {K,V} return nothing else el, i = itr - return inverse_hash(d, el)::K => d.map[el]::V, i + return d.inverse_hash(el)::K => d.map[el]::V, i end end -function inverse_hash(d::DenseDict{K, V, F, I}, el)::K where {K,V,F,I} - f = d.inverse_hash::I - return f(el)::K -end - Base.length(d::DenseDict) = length(d.set) Base.haskey(dict::DenseDict, key) = dict.hash(key) in dict.set Base.getindex(dict::DenseDict, key) = dict.map[dict.hash(key)] From 9ceded674094ba5f1057bce4f4fa2b203e5ed3e4 Mon Sep 17 00:00:00 2001 From: Joaquim Garcia Date: Thu, 6 Aug 2020 00:33:02 -0300 Subject: [PATCH 5/5] Add lazy behavior for dense dict --- src/Utilities/cachingoptimizer.jl | 22 ------- src/Utilities/copy.jl | 8 ++- src/Utilities/dense_dict.jl | 105 +++++++++++++++++++++++++----- test/Utilities/dense_dict.jl | 25 +++++++ 4 files changed, 119 insertions(+), 41 deletions(-) diff --git a/src/Utilities/cachingoptimizer.jl b/src/Utilities/cachingoptimizer.jl index cc9124803e..5781025e7c 100644 --- a/src/Utilities/cachingoptimizer.jl +++ b/src/Utilities/cachingoptimizer.jl @@ -184,28 +184,6 @@ function _standardize(d::AbstractDict{MOI.Index, MOI.Index}) return map end function _standardize(d::IndexMap) - # return d - # if we return d as is, its not possible to add variables - # if there was a dense dict inside - # the solution would be to allow automatically swtiching - # a ClevelDenseDict... - return IndexMap(_standard_dict(d.varmap), d.conmap) -end -function _standard_dict( - d::D -)::D where {D<:Dict{MOI.VariableIndex, MOI.VariableIndex}} - return d -end -function _standard_dict( - d::D -)::Dict{MOI.VariableIndex, MOI.VariableIndex} where { - D<:AbstractDict{MOI.VariableIndex, MOI.VariableIndex} -} - ret = Dict{MOI.VariableIndex, MOI.VariableIndex}() - sizehint!(ret, length(d)) - for (k,v) in d - ret[k] = v - end return d end diff --git a/src/Utilities/copy.jl b/src/Utilities/copy.jl index ee7bc7485b..eaf0073e93 100644 --- a/src/Utilities/copy.jl +++ b/src/Utilities/copy.jl @@ -78,12 +78,14 @@ function dense_variable_dict(::Type{V}, n) where V end struct IndexMap <: AbstractDict{MOI.Index, MOI.Index} + # cannot remove the standard dict without breaking varmap::Union{DenseVariableDict{MOI.VariableIndex}, - Dict{MOI.VariableIndex, MOI.VariableIndex}} + Dict{MOI.VariableIndex, MOI.VariableIndex}} conmap::DoubleDicts.MainIndexDoubleDict end -IndexMap() = IndexMap(Dict{MOI.VariableIndex, MOI.VariableIndex}(), - DoubleDicts.IndexDoubleDict()) +IndexMap() = IndexMap( + Dict{MOI.VariableIndex, MOI.VariableIndex}(), + DoubleDicts.IndexDoubleDict()) function IndexMap(n) IndexMap(dense_variable_dict(MOI.VariableIndex, n), DoubleDicts.IndexDoubleDict()) diff --git a/src/Utilities/dense_dict.jl b/src/Utilities/dense_dict.jl index c38d3489b3..837120042a 100644 --- a/src/Utilities/dense_dict.jl +++ b/src/Utilities/dense_dict.jl @@ -4,39 +4,112 @@ inverse_hash::I set::BitSet map::Vector{V} + dict::Dict{K,V} end -Same as `Dict{K, V}` but `hash(key)` is assumed to belong to `eachindex(map)`. +Same as `Dict{K, V}` but very performant if `hash(key)` belongs to +`eachindex(map)`. If `hash(key) == length(map) + 1` the dictionary +grows continuosly in aperformant fashion. Otherwise, a regular `Dict{K, V}` +is used. """ struct DenseDict{K, V, F, I} <: AbstractDict{K, V} hash::F inverse_hash::I set::BitSet map::Vector{V} - function DenseDict{K, V}(hash, inverse_hash, n) where {K, V} + dict::Dict{K,V} + function DenseDict{K, V}(hash, inverse_hash, n = 0) where {K, V} set = BitSet() sizehint!(set, n) - return new{K, V, typeof(hash), typeof(inverse_hash)}(hash, inverse_hash, set, Vector{K}(undef, n)) + return new{K, V, typeof(hash), typeof(inverse_hash)}( + hash, inverse_hash, set, Vector{K}(undef, n), Dict{K,V}() + ) end end +_is_dense(d::DenseDict) = !isempty(d.map) + # Implementation of the `AbstractDict` API. -# Base.empty(::DenseDict, ::Type{K}, ::Type{V}) not implemented +function Base.empty!(d::DenseDict) + if _is_dense(d) + empty!(d.set) + empty!(d.map) + else + empty!(d.dict) + end +end function Base.iterate(d::DenseDict{K,V}, args...) where {K,V} - itr = iterate(d.set, args...) - if itr === nothing - return nothing + if _is_dense(d) + itr = iterate(d.set, args...) + if itr === nothing + return nothing + else + el, i = itr + return d.inverse_hash(el)::K => d.map[el]::V, i + end + else + return Base.iterate(d.dict, args...) + end +end +function Base.length(d::DenseDict) + if _is_dense(d) + return length(d.set) + else + return length(d.dict) + end +end +function Base.haskey(d::DenseDict, key) + if _is_dense(d) + return d.hash(key) in d.set else - el, i = itr - return d.inverse_hash(el)::K => d.map[el]::V, i + return Base.haskey(d.dict, key) + end +end +function Base.getindex(d::DenseDict, key) + if _is_dense(d) + return d.map[d.hash(key)] + else + return d.dict[key] + end +end +function Base.setindex!(d::DenseDict, value, key) + h = d.hash(key) + if h <= length(d.map) && _is_dense(d) + push!(d.set, h) + d.map[h] = value + elseif h == length(d.map) + 1 && _is_dense(d) + push!(d.set, h) + push!(d.map, value) + else + if _is_dense(d) + _rehash(d) + end + d.dict[key] = value + end +end +function Base.sizehint!(d::DenseDict, n) + if _is_dense(d) + sizehint!(d.set, n) + sizehint!(d.map, n) + else + sizehint!(d.dict, n) end end -Base.length(d::DenseDict) = length(d.set) -Base.haskey(dict::DenseDict, key) = dict.hash(key) in dict.set -Base.getindex(dict::DenseDict, key) = dict.map[dict.hash(key)] -function Base.setindex!(dict::DenseDict, value, key) - h = dict.hash(key) - push!(dict.set, h) - dict.map[h] = value +function _rehash(d::DenseDict{K}) where K + sizehint!(d.dict, length(d.set)) + # assumes dict is currently dense + # iterator protocol from DenseDict is used + for (k,v) in d + d.dict[k] = v + end + empty!(d.set) + empty!(d.map) +end + +function Base.delete!(d::DenseDict{K}, k::K) where K + if _is_dense(d) + _rehash(d) + end + delete!(d.dict, k) end diff --git a/test/Utilities/dense_dict.jl b/test/Utilities/dense_dict.jl index 14edf1affe..1698142a06 100644 --- a/test/Utilities/dense_dict.jl +++ b/test/Utilities/dense_dict.jl @@ -32,3 +32,28 @@ d[6] = 0.75 @test d[2] == 1.5 @test d[4] == 0.25 @test d[6] == 0.75 + +d[8] = 1.75 + +sizehint!(d, 4) +@test Base.length(d) == 4 + +delete!(d, 6) +@test !haskey(d, 6) +@test d[2] == 1.5 +@test d[4] == 0.25 +@test d[8] == 1.75 +@test Base.length(d) == 3 + +sizehint!(d, 4) +d[24] = 2.5 +@test d[24] == 2.5 + +@test sort(collect(d)) == sort([2 => 1.5, 4 => 0.25, 8 => 1.75, 24 => 2.5]) + +empty!(d) +@test length(d) == 0 + +d = MOI.Utilities.DenseDict{Int, Float64}(div2, mul2, 3) +empty!(d) +@test length(d) == 0