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

Extremely long delay and huge allocation defining convert #20671

Closed
yuyichao opened this issue Feb 19, 2017 · 2 comments
Closed

Extremely long delay and huge allocation defining convert #20671

yuyichao opened this issue Feb 19, 2017 · 2 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version
Milestone

Comments

@yuyichao
Copy link
Contributor

yuyichao commented Feb 19, 2017

Accidentally noticed that using SIUnits was using more than half of the memory I have.

Reduced to the following file.

__precompile__()

module M

immutable SIQ{T<:Number,m,kg,s,A,K,mol,cd,rad,sr} <: Number
    val::T
end

function Base.convert{T,S,mS,kgS,sS,AS,KS,molS,cdS,radS,srS,mT,kgT,sT,AT,KT,molT,cdT,radT,srT}(::Type{SIQ{T,mT,kgT,sT,AT,KT,molT,cdT,radT,srT}}, val::SIQ{S,mS,kgS,sS,AS,KS,molS,cdS,radS,srS})
    SIQ{T,mT,kgT,sT,AT,KT,molT,cdT,radT,srT}(convert(T, val.val))
end

end # module

When running this from REPL, the method definition takes ~30s to complete with 16+G of allocation. With precompilation (put this in a file M.jl, add that to JULIA_LOAD_PATH and run using M), the loading of the precompiled file takes equally long and has similar allocations that is not free'd, causing a 16-17G peak memory consumption (see memory usage graph below, this is on a system with 32G of memory).

spectacle l16052

Seems like there are a few problems here.

  1. Some type inference/dispatch heuristic might be broken causing us to do much more work than necessary
  2. Somehow that work is done twice for precompilation (once on the precompile worker process and once on the loader)
  3. In the precompile loader, that works seems to be done with the GC turned off. Causing the memory usage to sky-rocket at ~0.5G/s....

Adding to milestone since this is clearly a regression. We can remove this from the milestone after this is partially fixed if necessary.

@yuyichao yuyichao added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version labels Feb 19, 2017
@yuyichao yuyichao added this to the 0.6.0 milestone Feb 19, 2017
@yuyichao
Copy link
Contributor Author

Actually it seems that this is all due to a single type intersection in the ambiguity detection....

julia> immutable SIQ{T<:Number,m,kg,s,A,K,mol,cd,rad,sr} <: Number
           val::T
       end

julia> a = Tuple{Type{SIQ{T, mT, kgT, sT, AT, KT, molT, cdT, radT, srT}}, SIQ{S, mS, kgS, sS, AS, KS, molS, cdS, radS, srS}} where srT where radT where cdT where molT where KT where AT where sT where kgT where mT where srS where radS where cdS where molS where KS where AS where sS where kgS where mS where S where T
Tuple{Type{SIQ{T,mT,kgT,sT,AT,KT,molT,cdT,radT,srT}},SIQ{S,mS,kgS,sS,AS,KS,molS,cdS,radS,srS}} where srT where radT where cdT where molT where KT where AT where sT where kgT where mT where srS where radS where cdS where molS where KS where AS where sS where kgS where mS where S where T

julia> b = Tuple{Type{T}, T} where T
Tuple{Type{T},T} where T

julia> typeintersect(a, b)
Tuple{Type{SIQ{T,mS,kgS,sS,AS,KS,molS,cdS,radS,srS}},SIQ{T,mS,kgS,sS,AS,KS,molS,cdS,radS,srS}} where srS where radS where cdS where molS where KS where AS where sS where kgS where mS where T

julia> @time typeintersect(a, b)
 30.471441 seconds (240.14 M allocations: 16.675 GiB, 4.19% gc time)
Tuple{Type{SIQ{T,mS,kgS,sS,AS,KS,molS,cdS,radS,srS}},SIQ{T,mS,kgS,sS,AS,KS,molS,cdS,radS,srS}} where srS where radS where cdS where molS where KS where AS where sS where kgS where mS where T

So I guess removing it from the precompile loading could be hard. typeintersect should probably be faster on this and maybe we can delay the method insertion/ambiguity detection after we turn the GC back on?

@JeffBezanson
Copy link
Member

+1 to turning the GC on sooner. Of course we need to try to improve type intersection here too.

@JeffBezanson JeffBezanson self-assigned this Mar 27, 2017
JeffBezanson added a commit that referenced this issue Mar 28, 2017
@iamnapo iamnapo mentioned this issue Apr 5, 2017
vtjnash added a commit that referenced this issue Jun 12, 2017
vtjnash added a commit that referenced this issue Jun 12, 2017
vtjnash added a commit that referenced this issue Jun 13, 2017
vtjnash added a commit that referenced this issue Jun 14, 2017
vtjnash added a commit that referenced this issue Jun 15, 2017
the previous attempt to preserve the backedge map was being too cute
it is more reliable to simply flatten the whole map into the new caller
also corrects the validation that the backedge is not already invalid

make sure internal backedges are only internal:
this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete
that any backedges encountered there are purely internal / new

enable GC slightly sooner when loading precompiled modules (part of #20671)
reverts 11a984b - but who needed that anyways
vtjnash added a commit that referenced this issue Jun 15, 2017
the previous attempt to preserve the backedge map was being too cute
it is more reliable to simply flatten the whole map into the new caller
also corrects the validation that the backedge is not already invalid

make sure internal backedges are only internal:
this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete
that any backedges encountered there are purely internal / new

enable GC slightly sooner when loading precompiled modules (part of #20671)
reverts 11a984b - but who needed that anyways

(cherry-picked from 6fdf36e, PR #22340)
vtjnash added a commit that referenced this issue Jun 15, 2017
the previous attempt to preserve the backedge map was being too cute
it is more reliable to simply flatten the whole map into the new caller
also corrects the validation that the backedge is not already invalid

make sure internal backedges are only internal:
this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete
that any backedges encountered there are purely internal / new

enable GC slightly sooner when loading precompiled modules (part of #20671)
reverts 11a984b - but who needed that anyways

(cherry-picked from 0e19311, PR #22340)
quinnj pushed a commit that referenced this issue Jun 20, 2017
the previous attempt to preserve the backedge map was being too cute
it is more reliable to simply flatten the whole map into the new caller
also corrects the validation that the backedge is not already invalid

make sure internal backedges are only internal:
this preserves the expected invariant for jl_insert_backedges/jl_method_instance_delete
that any backedges encountered there are purely internal / new

enable GC slightly sooner when loading precompiled modules (part of #20671)
reverts 11a984b - but who needed that anyways
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

2 participants