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

Use Dict constructor for 1 and 2 arguments new_ordered_dict #1640

Merged
merged 1 commit into from
Nov 20, 2018

Conversation

blegat
Copy link
Member

@blegat blegat commented Nov 20, 2018

Based on the following benchmark:

function slow_new_ordered_dict(::Type{K}, ::Type{V}, kv::Pair...) where {K,V}
    dict = OrderedDict{K,V}()
    sizehint!(dict, length(kv))
    for pair in kv
        JuMP.add_or_set!(dict, convert(K, pair.first), convert(V, pair.second))
    end
    return dict
end
function fast_new_ordered_dict(::Type{K}, ::Type{V}, kv::Pair) where {K,V}
    return OrderedDict{K,V}(kv)
end
function fast_new_ordered_dict(::Type{K}, ::Type{V}, kv1::Pair, kv2) where {K,V}
    if isequal(kv1.first, kv2.first)
        return OrderedDict{K,V}(kv1.first => kv1.second + kv2.second)
    else
        return OrderedDict{K,V}(kv1, kv2)
    end
end
using BenchmarkTools
using JuMP
function bench()
    model = Model()
    a = 2.0
    @variable(model, x)
    b = -1.0
    @variable(model, y)
    @btime slow_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a)
    @btime fast_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a)
    @btime slow_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a, $y => $b)
    @btime fast_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a, $y => $b)
    @btime slow_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a, $x => $b)
    @btime fast_new_ordered_dict(JuMP.VariableRef, Float64, $x => $a, $x => $b)
end
bench()

which gives

  266.433 ns (8 allocations: 624 bytes)
  233.452 ns (7 allocations: 592 bytes)
  324.908 ns (9 allocations: 656 bytes)
  291.885 ns (9 allocations: 656 bytes)
  313.509 ns (9 allocations: 656 bytes)
  249.245 ns (9 allocations: 656 bytes)

The speedup is smaller than I would expect but it is still noticeable.

The timing with speed.jl is:

P-Median(100 facilities, 100 customers, 5000 locations) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  714.37 MiB
  allocs estimate:  12030567
  --------------
  minimum time:     1.779 s (51.12% GC)
  median time:      1.941 s (52.86% GC)
  mean time:        2.082 s (56.42% GC)
  maximum time:     2.527 s (62.89% GC)
  --------------
  samples:          3
  evals/sample:     1
Cont5(n=500) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  402.77 MiB
  allocs estimate:  6785839
  --------------
  minimum time:     849.008 ms (39.81% GC)
  median time:      955.320 ms (42.55% GC)
  mean time:        951.107 ms (43.90% GC)
  maximum time:     1.056 s (48.26% GC)
  --------------
  samples:          6
  evals/sample:     1

which is a slight improvement compared to the timing reported in #1639

@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

Merging #1640 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1640      +/-   ##
==========================================
+ Coverage   90.85%   90.86%   +<.01%     
==========================================
  Files          28       28              
  Lines        3751     3755       +4     
==========================================
+ Hits         3408     3412       +4     
  Misses        343      343
Impacted Files Coverage Δ
src/aff_expr.jl 94.95% <100%> (+0.17%) ⬆️

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 616ad67...7c491c6. Read the comment docs.

@blegat blegat merged commit 2db350a into master Nov 20, 2018
@blegat blegat deleted the bl/new_ordered_dict branch December 12, 2018 17:05
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.

None yet

1 participant