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

⚡️ Only track variables indices once one is deleted #568

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

blegat
Copy link
Member

@blegat blegat commented Nov 12, 2018

If we didn't have to support deleting variables, we could just implement add_variables as follows and get +- 2ns and 0 allocation for creating one variable.

using MathOptInterface
const MOI = MathOptInterface

MOI.Utilities.@model(Model, (), (), (), (), (), (), (), ())
const model = Model{Float64}();
_add_variable(model) = MOI.VariableIndex(model.num_variables_created += 1)
julia> @btime _add_variable(model)
  1.814 ns (0 allocations: 0 bytes)

However, because some variables can be deleted we keep a Set{MOI.VariableIndex} of the variables and everytime one is created, we add it to the set.
This means that when adding a variable, we need to compute its hash, and we sometime need to allocate to resize the memory used by the set.
We can see that the performance is +- 20 slower and it gets even slower if we keep calling MOI.add_variable because when the set is resized, it allocates even more memory

julia> @btime MOI.add_variable(model)
  45.070 ns (0 allocations: 0 bytes)
julia> @btime MOI.add_variable(model)
  66.451 ns (0 allocations: 0 bytes)
julia> @btime MOI.add_variable(model)
  72.154 ns (0 allocations: 0 bytes)

It seems a bit unfortunate to do all this if the user is never going to delete a variable. Indeed, in that case the indices are simply MOI.VariableIndex.(1:n) where n is the number of variables created. So we only need to keep track of n.
A similar reasoning lead to only create the dictionary for looking up by names only when the first lookup is queried : #535

We follow a similar approach in this PR and only create this Set when a variable is deleted.
This gives a 10x speedup in time and ∞ speedup in memory.

julia> @btime MOI.add_variable(model)
4.038 ns (0 allocations: 0 bytes)

@codecov-io
Copy link

codecov-io commented Nov 12, 2018

Codecov Report

Merging #568 into master will decrease coverage by 14.33%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #568       +/-   ##
===========================================
- Coverage   95.67%   81.34%   -14.34%     
===========================================
  Files          47       47               
  Lines        4834     4814       -20     
===========================================
- Hits         4625     3916      -709     
- Misses        209      898      +689
Impacted Files Coverage Δ
src/Utilities/model.jl 86.86% <100%> (-13.14%) ⬇️
src/Bridges/Bridges.jl 18.18% <0%> (-81.82%) ⬇️
src/Test/UnitTests/solve.jl 28.12% <0%> (-71.88%) ⬇️
src/Bridges/singlebridgeoptimizer.jl 40% <0%> (-60%) ⬇️
src/Test/UnitTests/basic_constraint_tests.jl 48% <0%> (-50.08%) ⬇️
src/Bridges/squarepsdbridge.jl 56.45% <0%> (-41.94%) ⬇️
src/Bridges/geomeanbridge.jl 59.67% <0%> (-40.33%) ⬇️
src/variables.jl 60% <0%> (-40%) ⬇️
src/Bridges/rsocbridge.jl 62.06% <0%> (-37.94%) ⬇️
src/Bridges/quadtosocbridge.jl 54.54% <0%> (-37.88%) ⬇️
... and 34 more

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 cfce251...cd27b0b. Read the comment docs.

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

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

Nice! There's also a lot of extra overhead in the SingleVariable constraints compared with setting variable categories and bounds as we did before.

@blegat blegat merged commit b42f03c into master Nov 13, 2018
@odow odow deleted the bl/fast_nodelvar branch March 6, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants