-
Notifications
You must be signed in to change notification settings - Fork 0
Description
If you run performance_test.jl, you'll see that the first run of build! take almost 2 minutes, and the second run takes merely 2 seconds. Much of that extra time is spent doing type inference, which says that our code has bad type stability issues. I think I've identified the culprit: it's the _add_foo_container! functions. Namely, we do:
function _add_variable_container!(
container::OptimizationContainer,
...;
sparse::Bool,
) where {T <: VariableType, U <: Union{PSY.Component, PSY.System}}
if sparse
var_container = sparse_container_spec(JuMP.VariableRef, axs...)
else
var_container = container_spec(JuMP.VariableRef, axs...)
end
_assign_container!(container.variables, var_key, var_container)
end
function _add_param_container!(
container::OptimizationContainer,
....
)
if built_for_recurrent_solves(container) && !get_rebuild_model(get_settings(container))
param_type = JuMP.VariableRef
else
param_type = Float64
end
# add param container with that type
endTypes of core containers depend on runtime values: that's bad type instability. Steps that would help:
- Make
rebuild_modela type parameter ofSettings. Similarly,OptimizationContainershould be parameterized onbuilt_for_recurrent_solvesand onrebuild_model, so that theifbranch can be deduced at compile time.DecisionModelpasses itsbuilt_for_recurrent_solvesfield along toOptimizationContainer, so that one would likely need to become a type parameter too. - Change
built_for_recurrent_solvesandsparseto positional arguments. Julia does not specialize or dispatch on keyword arguments. Looking through the code, it looks like we always know the value ofsparseat compile time (it's never provided by the user), so we could even go one step further and useVal{true}andVal{False}.
The first item above should be pretty doable, as long as we're ok with always providing built_for_recurrent_solves upon initialization and its value remaining the same over the lifetime of the object. The second item breaks backwards compatibility--it'd change the public interface of DecisionModel--so that's more drastic.