Improved constraint deletion performance by separating functions #95

ndinsmore · 2019-02-26T18:24:20Z

This change improves the performance of constraint deletion by more than 6x.

Using the following benchmark:

using JuMP
using GLPK
using Clp
using BenchmarkTools

function constraint_deletion_benchmark(opt_package,num_variables,solver_opts=NamedTuple())
    model = Model(with_optimizer(opt_package.Optimizer; solver_opts...))

    vars=@variable(model,[i = 1:num_variables])

    cons=@constraint(model,vars .<= 1)

    for con in cons
        delete(model,con)
    end

    cons=@constraint(model,vars .<= 1)
    @objective(model,Max,sum(vars))
    optimize!(model)

    for con in cons
        delete(model,con)
    end
end

#------
@benchmark constraint_deletion_benchmark(GLPK,1000)
@benchmark constraint_deletion_benchmark(Clp,1000,(LogLevel=0,))

master:

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  78.52 MiB
  allocs estimate:  3852740
  --------------
  minimum time:     295.216 ms (4.87% GC)
  median time:      327.834 ms (4.88% GC)
  mean time:        328.450 ms (6.63% GC)
  maximum time:     416.230 ms (28.00% GC)
  --------------
  samples:          16
  evals/sample:     1

julia> @benchmark constraint_deletion_benchmark(Clp,1000,(LogLevel=0,))
BenchmarkTools.Trial: 
  memory estimate:  78.94 MiB
  allocs estimate:  3854763
  --------------
  minimum time:     289.888 ms (5.43% GC)
  median time:      323.338 ms (4.64% GC)
  mean time:        326.543 ms (6.63% GC)
  maximum time:     423.405 ms (28.99% GC)
  --------------
  samples:          16
  evals/sample:     1

ndinsmore:row_deletion_perf:

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  5.98 MiB
  allocs estimate:  118130
  --------------
  minimum time:     40.884 ms (0.00% GC)
  median time:      50.572 ms (0.00% GC)
  mean time:        60.356 ms (2.37% GC)
  maximum time:     231.075 ms (3.69% GC)
  --------------
  samples:          83
  evals/sample:     1

julia> @benchmark constraint_deletion_benchmark(Clp,1000,(LogLevel=0,))
BenchmarkTools.Trial: 
  memory estimate:  6.41 MiB
  allocs estimate:  120153
  --------------
  minimum time:     30.155 ms (0.00% GC)
  median time:      43.865 ms (0.00% GC)
  mean time:        57.698 ms (3.26% GC)
  maximum time:     515.472 ms (1.47% GC)
  --------------
  samples:          87
  evals/sample:     1

odow

Thanks! The first version of LQOI wasn't written for performance so there are probably a few of these type of things that could be improved.

src/LinQuadOptInterface.jl

odow

The latest changes still rely on accessing the internal .vals field of a dictionary.

Can you benchmark against a naive implementation of:

for (key, value) in scalar
    if value > row
        scalar[key] -= 1
    end
end

Is this really the performance bottleneck when deleting constraints? If so, is this the right fix? Or can we make a larger restructure to sort things out.

ndinsmore · 2019-02-26T22:35:05Z

for (key,val) in scalar
        scalar[key] = val > row ? val - 1 : val
end

yields

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  13.62 MiB
  allocs estimate:  618630
  --------------
  minimum time:     91.325 ms (0.00% GC)
  median time:      100.139 ms (2.98% GC)
  mean time:        101.903 ms (2.03% GC)
  maximum time:     128.013 ms (2.51% GC)
  --------------
  samples:          50
  evals/sample:     1

for (key, value) in scalar
    if value > row
        scalar[key] -= 1
    end
end

yields

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  21.23 MiB
  allocs estimate:  1117130
  --------------
  minimum time:     139.537 ms (2.12% GC)
  median time:      148.302 ms (1.99% GC)
  mean time:        151.534 ms (1.93% GC)
  maximum time:     173.682 ms (2.35% GC)
  --------------
  samples:          33
  evals/sample:     1

codecov-io · 2019-02-26T22:35:10Z

Codecov Report

Merging #95 into master will decrease coverage by 3.5%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   90.29%   86.78%   -3.51%     
==========================================
  Files          12       12              
  Lines        1339     1355      +16     
==========================================
- Hits         1209     1176      -33     
- Misses        130      179      +49

Impacted Files	Coverage Δ
src/LinQuadOptInterface.jl	`96.29% <100%> (-2.12%)`	⬇️
src/solver_interface.jl	`25% <0%> (-41.67%)`	⬇️
src/objective.jl	`75.28% <0%> (-6.75%)`	⬇️
src/constraints.jl	`91.3% <0%> (-4.35%)`	⬇️
src/mockoptimizer.jl	`80.15% <0%> (-3.52%)`	⬇️
src/constraints/scalaraffine.jl	`92.36% <0%> (-3.42%)`	⬇️
src/variables.jl	`96.05% <0%> (-2.64%)`	⬇️
src/constraints/singlevariable.jl	`94.36% <0%> (-2.09%)`	⬇️
src/constraints/vectoraffine.jl	`93.61% <0%> (-2.09%)`	⬇️
... and 1 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 82246f7...0c73fee. Read the comment docs.

joaquimg · 2019-02-27T02:02:09Z

Fast deleting is great 6x looks a important improvement.
Looks like a bottleneck.
If someone has a better data structure to propose if would be great.
Otherwise accessing this internal field is bad but not that bad...
Are there built-in Dict functions to it?

odow · 2019-02-27T14:27:42Z

What about

for (key, value) in scalar
    if value > row
        scalar[key] = value - 1
    end
end

This avoids the extra lookup associated with the -=.

We've gone from 300ms to 100ms (Great!) (and probably even a little less if we have to perform less setindex! above). I don't think accessing the .vals field of a dict offsets the <40ms gain we get.

If this is a performance issue, I would regard this as a problem in Base Julia. It should provide an exported, documented function that allows querying and setting a key-value pair without having to perform multiple lookups.

ndinsmore · 2019-02-27T20:47:52Z

I have asked this basic question on discourse. But some investigation shows that the naive solution really starts to fall off above 1000 elements.

joaquimg · 2019-03-05T18:53:41Z

@ndinsmore can cross reference the discourse post?

ndinsmore · 2019-03-05T21:44:37Z

This is the discourse post:
https://discourse.julialang.org/t/fast-dict-value-modification-by-accessing-dict-vals/21277/10

Which has evolved into the Julia PR: JuliaLang/julia#31223

ndinsmore · 2019-03-06T14:32:02Z

It looks like the PR is gaining enough momentum that it will eventually be added. In the mean time should we implement the naive version?

odow · 2019-03-06T15:31:37Z

Great, thanks for escalating this to Base Julia. Let's implement the naive loop version with a TODO linking to the Julia PR.

ndinsmore · 2019-03-14T21:17:10Z

@odow the PR has been merged with master: JuliaLang/julia#31223
I have update this repo to include the definition of map! then to also make use of it.
The code for map! is the same as that in julia master

joaquimg · 2019-03-14T22:40:21Z

Slightly related comment:
There is a related open issue JuliaLang/julia#31199
That could also benefit JuMP if you are interested.

ndinsmore · 2019-03-19T19:09:03Z

Is there anything that is blocking this PR?

odow · 2019-03-19T22:42:46Z

CI is failing. Can we see a before and after benchmark one last time please?

ndinsmore · 2019-03-20T20:31:18Z

It is now closer to 10x faster
Master:

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  78.49 MiB
  allocs estimate:  3863538
  --------------
  minimum time:     301.017 ms (4.38% GC)
  median time:      337.761 ms (4.04% GC)
  mean time:        350.259 ms (5.62% GC)
  maximum time:     444.934 ms (24.12% GC)
  --------------
  samples:          15
  evals/sample:     1

julia> @benchmark constraint_deletion_benchmark(Clp,1000,(LogLevel=0,))
BenchmarkTools.Trial: 
  memory estimate:  78.72 MiB
  allocs estimate:  3863560
  --------------
  minimum time:     292.421 ms (3.75% GC)
  median time:      320.846 ms (4.76% GC)
  mean time:        377.702 ms (8.67% GC)
  maximum time:     871.552 ms (29.00% GC)
  --------------
  samples:          14
  evals/sample:     1

row_deletion_perf:

julia> @benchmark constraint_deletion_benchmark(GLPK,1000)
BenchmarkTools.Trial: 
  memory estimate:  5.74 MiB
  allocs estimate:  115228
  --------------
  minimum time:     37.724 ms (0.00% GC)
  median time:      42.170 ms (0.00% GC)
  mean time:        46.151 ms (4.75% GC)
  maximum time:     183.727 ms (0.00% GC)
  --------------
  samples:          109
  evals/sample:     1

julia> @benchmark constraint_deletion_benchmark(Clp,1000,(LogLevel=0,))
BenchmarkTools.Trial: 
  memory estimate:  5.97 MiB
  allocs estimate:  115250
  --------------
  minimum time:     24.873 ms (0.00% GC)
  median time:      29.140 ms (0.00% GC)
  mean time:        30.183 ms (4.00% GC)
  maximum time:     51.206 ms (10.70% GC)
  --------------
  samples:          166
  evals/sample:     1

odow · 2019-03-21T13:25:29Z

src/LinQuadOptInterface.jl

+        dict = iter.dict
+        vals = dict.vals
+        # @inbounds is here so the it gets propigated to isslotfiled
+        @inbounds for i = dict.idxfloor:Base.lastindex(vals)


This is causing an issue on Julia 0.6

If we are dropping support for v0.6 does it matter?

src/LinQuadOptInterface.jl

odow · 2019-04-11T00:16:20Z

Merged despite the failing tests on 0.6 because I just merged a PR bumping Julia to >=1.0.

odow · 2019-04-11T00:16:32Z

Sorry this took so long.

Improved constraint deletion performance by seperating functions

ab67201

odow reviewed Feb 26, 2019

View reviewed changes

src/LinQuadOptInterface.jl Outdated Show resolved Hide resolved

src/LinQuadOptInterface.jl Outdated Show resolved Hide resolved

src/LinQuadOptInterface.jl Outdated Show resolved Hide resolved

Fix comments and add for loop equivilent for clarity.

a1bea6d

odow reviewed Feb 26, 2019

View reviewed changes

src/LinQuadOptInterface.jl Outdated Show resolved Hide resolved

src/LinQuadOptInterface.jl Outdated Show resolved Hide resolved

ndinsmore added 4 commits February 26, 2019 16:09

Spelling

3772a14

Change to for loops for clarity

fc346a4

Better comments

3f9adba

Fixed test

bf9d6db

odow reviewed Feb 26, 2019

View reviewed changes

Assert test for struct type

2052dba

Change to make use of map!

c42a8de

test conflict

8f113fc

Merge remote-tracking branch 'origin/master' into row_deletion_perf

576fcaa

odow reviewed Mar 21, 2019

View reviewed changes

ndinsmore mentioned this pull request Apr 9, 2019

Lazy variables dict after delete. #97

Open

Added comments and improved deleteref! method

0c73fee

odow merged commit 4769deb into JuliaOpt:master Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved constraint deletion performance by separating functions #95

Improved constraint deletion performance by separating functions #95

ndinsmore commented Feb 26, 2019 •

edited

Loading

odow left a comment

odow left a comment

ndinsmore commented Feb 26, 2019

codecov-io commented Feb 26, 2019 •

edited

Loading

joaquimg commented Feb 27, 2019

odow commented Feb 27, 2019

ndinsmore commented Feb 27, 2019

joaquimg commented Mar 5, 2019

ndinsmore commented Mar 5, 2019

ndinsmore commented Mar 6, 2019

odow commented Mar 6, 2019

ndinsmore commented Mar 14, 2019

joaquimg commented Mar 14, 2019

ndinsmore commented Mar 19, 2019

odow commented Mar 19, 2019

ndinsmore commented Mar 20, 2019 •

edited

Loading

odow Mar 21, 2019

ndinsmore Apr 10, 2019

odow commented Apr 11, 2019

odow commented Apr 11, 2019

Improved constraint deletion performance by separating functions #95

Improved constraint deletion performance by separating functions #95

Conversation

ndinsmore commented Feb 26, 2019 • edited Loading

odow left a comment

Choose a reason for hiding this comment

odow left a comment

Choose a reason for hiding this comment

ndinsmore commented Feb 26, 2019

codecov-io commented Feb 26, 2019 • edited Loading

Codecov Report

joaquimg commented Feb 27, 2019

odow commented Feb 27, 2019

ndinsmore commented Feb 27, 2019

joaquimg commented Mar 5, 2019

ndinsmore commented Mar 5, 2019

ndinsmore commented Mar 6, 2019

odow commented Mar 6, 2019

ndinsmore commented Mar 14, 2019

joaquimg commented Mar 14, 2019

ndinsmore commented Mar 19, 2019

odow commented Mar 19, 2019

ndinsmore commented Mar 20, 2019 • edited Loading

odow Mar 21, 2019

Choose a reason for hiding this comment

ndinsmore Apr 10, 2019

Choose a reason for hiding this comment

odow commented Apr 11, 2019

odow commented Apr 11, 2019

ndinsmore commented Feb 26, 2019 •

edited

Loading

codecov-io commented Feb 26, 2019 •

edited

Loading

ndinsmore commented Mar 20, 2019 •

edited

Loading