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

DNMY: Tests for performance improvements #2629

Closed
wants to merge 2 commits into from
Closed

DNMY: Tests for performance improvements #2629

wants to merge 2 commits into from

Conversation

odow
Copy link
Member

@odow odow commented Jun 16, 2021

Opening this PR so @jd-lara has a target to benchmark against.

Closes #2628

@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #2629 (87d1bad) into master (2ff880d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2629   +/-   ##
=======================================
  Coverage   93.50%   93.51%           
=======================================
  Files          44       44           
  Lines        5545     5547    +2     
=======================================
+ Hits         5185     5187    +2     
  Misses        360      360           
Impacted Files Coverage Δ
src/aff_expr.jl 95.88% <100.00%> (+0.04%) ⬆️

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 2ff880d...87d1bad. Read the comment docs.

@jd-lara
Copy link
Contributor

jd-lara commented Jun 17, 2021

Some results of building the model: The Line Section is the one we are trying to improve. This is the result of a "sparse" expression array where there are 0's. If the ExpressionArray is filled with DoubleExpressions the problem never builds.

Julia tagged version*

image

This Branch

image

@jd-lara
Copy link
Contributor

jd-lara commented Jun 17, 2021

@odow one of the things to note here is that when we do constant*ParameterJuMP.DoubleGenericAffExpr{Float64, VariableRef, ParameterRef}

This goes into this function defined in ParameterJumP.jl

Base.:(*)(lhs::Number, rhs::PAE{C}) where C<:Number = PAE{C}(lhs*rhs.v, lhs*rhs.p)

Which in turn calls this code twice for the same lhs value as a result map_coefficients(c -> α * c, rhs) gets an anonymous function twice for the same parameter and performs the same operation on the other expression.

In *(lhs, rhs) at /Users/jdlara/.julia/packages/JuMP/7JcAM/src/operators.jl:50
 50  function Base.:*(lhs::_Constant, rhs::_GenericAffOrQuadExpr)
>51      if iszero(lhs)
 52          return zero(rhs)
 53      else
 54          α = _constant_to_number(lhs)
 55          return map_coefficients(c -> α * c, rhs)

@odow
Copy link
Member Author

odow commented Jun 17, 2021

Only if called outside the macro. JuMP rewrites ret += lhs * rhs to add_to_expression!(ret, lhs, rhs)

@jd-lara
Copy link
Contributor

jd-lara commented Jun 17, 2021

I am running debugger, so this is what is being called in the @expression macro

@odow
Copy link
Member Author

odow commented Jun 17, 2021

Closing this for now: JuliaStochOpt/ParameterJuMP.jl#85 (comment)

I finally tracked down the likely culprit: Julia allocates ScalarConstraint(::AffExpr,::EqualTo) to the stack, but ScalarConstraint(::ParameterJuMP.DoubleGenericAffExpr{Float64, VariableRef, ParameterRef}, ::EqualTo) to the heap.

Cue more allocations and GC time. These allocations aren't coming from a mutable arithmetic failure.

@odow odow closed this Jun 17, 2021
@odow odow deleted the od/jose branch June 17, 2021 05:08
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.

Affine expressions are slow
2 participants