search

JuliaApproximation / ApproxFunBase.jl

Core functionality of ApproxFun
MIT License
12 stars 13 forks source link

iszero type piracy causes a lot of invalidations #51

Closed KristofferC closed 4 years ago

KristofferC commented 4 years ago

The definition here:

https://github.com/JuliaApproximation/ApproxFunBase.jl/blob/663770723cea962efe6834d250a7f2ce90680289/src/Fun.jl#L506

changes the definition of iszero for all numbers (type piracy) and causes a huge number of invalidations 

 inserting iszero(x::Number) in ApproxFunBase at /home/kc/.julia/packages/ApproxFunBase/r0W9A/src/Fun.jl:506 invalidated:
   backedges: 1: superseding iszero(x) in Base at number.jl:40 with MethodInstance for iszero(::UInt32) (1 children)
              2: superseding iszero(x) in Base at number.jl:40 with MethodInstance for iszero(::UInt64) (2 children)
              3: superseding iszero(x) in Base at number.jl:40 with MethodInstance for iszero(::Int64) (6236 children) <---------------

You can see the effect of this by loading ApproxFunBase and notice that the whole julia session becomes sluggish while it is recompiling everything.

dlfivefifty commented 4 years ago

Thanks! That’s a bizarre one how that got there

KristofferC commented 4 years ago

Seems to have come from https://github.com/JuliaApproximation/ApproxFun.jl/pull/488/files#diff-315609d5896fe40b083c84a9432a2361R444.

dlfivefifty commented 4 years ago

Thanks! It looks like I didn't import iszero when I added the function (so was meant to be ApproxFun.iszero), and later added import Base: iszero.

I'll fix it now.

KristofferC commented 4 years ago

Never use import Base: foo :P

dlfivefifty commented 4 years ago

I always use import Base: foo 🤣 That way when Base: foo becomes LinearAlgebra: foo its a one-line change.

dlfivefifty commented 4 years ago

I'm also following the official convention advocated by StdLib: https://github.com/JuliaLang/julia/blob/395e47f4da4630a521333ad67eccf614c56aaf9c/stdlib/LinearAlgebra/src/LinearAlgebra.jl#L11

KristofferC commented 4 years ago

I'm also following the official convention advocated by StdLib:

Most of the code in the stdlibs were written at a time when people didn't know better :rofl:

dlfivefifty commented 4 years ago

I think introspection tools are a better way to go, say a GitHub action that does whatever it is you did to find these types of errors. Expecting developer style (and more importantly refactoring 100k's of existing Julia code) to prevent this is unrealistic.

KristofferC commented 4 years ago

Accidental method extension is a problem So I stand by my advice to not use a way of coding which makes that easy.

dlfivefifty commented 4 years ago

You might be right, but it's probably easier to get Julia to spit out a warning if a package overloads functions with types that do not belong to that package than it is to re-write every single Julia package in that style...

(Yes I'm aware it's not so simple because of "sanctioned" overloading like OffsetArrays.jl, but I'm sure some macro could be introduced or something to stop the warning.)

timholy commented 4 years ago

We're about to announce a competition on discourse...shhh, don't tell, this one is the current record-holder for the largest number of invalidations caused by a single method definition. A whopping 13% of all MethodInstances on Julia 1.6!

No one who has read this is allowed to submit it, that honor goes to @KristofferC.

dlfivefifty commented 4 years ago

I had no idea back in 2014 that my hacked together package would go on to cause so much damage to the Julia ecosystem... (And I guess given energy usage, the real ecosystem as well!) 🤯

timholy commented 4 years ago

No worries, we've all learned a lot in the last few months about this stuff.

timholy commented 4 years ago

ImageCore had these: https://github.com/JuliaImages/ImageCore.jl/pull/131. A mere 200ish invalidations, so I bow before your greatness, but no better from a piracy perspective.

dlfivefifty commented 4 years ago

The worst part is I've spent literally weeks trying to debug why ApproxFun.jl loading was so slow via code-bisection.... was probably very close to finding this culprit

timholy commented 4 years ago

How much did it change with this PR?

dlfivefifty commented 4 years ago

Oh actually not that much, 15.1s v 15.3s...

KristofferC commented 4 years ago

I think the biggest difference will be in sluggishness in the REPL and package manager etc after loading the package.

KristofferC commented 4 years ago

Also, even with this fixed there is still a large number of invalidations going on (e.g. https://github.com/invenia/Intervals.jl/issues/144).

timholy commented 4 years ago

Yeah, in general when you fix an invalidation problem in PkgA, it's PkgB (which depends on PkgA) that will thank you. So if you're still thinking this package is really slow to load, start using @snoopr on the packages it depends on.

dlfivefifty commented 4 years ago

I’m in the process of completely redesigning it via ContinuumArrays.jl which is much snappier already, so I won’t be looking into it