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

defining a few constructor methods gives 2x import slowdown #27599

Open
stevengj opened this issue Jun 15, 2018 · 5 comments
Open

defining a few constructor methods gives 2x import slowdown #27599

stevengj opened this issue Jun 15, 2018 · 5 comments
Labels
compiler:latency Compiler latency

Comments

@stevengj
Copy link
Member

stevengj commented Jun 15, 2018

In updating PyCall for the latest Julia (JuliaPy/PyCall.jl#505), I noticed that dealing with #23273 led to a huge slowdown.

In particular, PyCall used to define only convert(T, o::PyObject) methods for converting Python objects to native Julia types, and I got T(o) constructors automatically. In updating, I first tried changing all of my convert methods to constructors, but this led to 30x (!) load-time slowdowns (after precompiling) on 0.6, and out-of-memory errors in Travis. Instead, I opted to leave the convert methods as-is.

What I noticed is that, if I also add an explicit constructor that calls the convert method, to preserve the constructor API on 0.7, it led to a big slowdown in load time. In particular, just adding

if VERSION >= v"0.7.0-DEV.3152" # julia#23273
    (::Type{T})(po::PyObject) where
        {T<:Union{Number,Nothing,AbstractString,Symbol,Array,Ptr{Cvoid},
        Function,Tuple,Pair,Dict,AbstractRange,Dates.AbstractTime}} =
        convert(T, po)
end

(at the end of PyCall/src/conversions.jl in my JuliaPy/PyCall.jl#505 branch) caused the using PyCall time (after precompiling) to slow down from 10s to 20s on my machine. (And it is 3.5s on 0.6, so there are other slowdowns too.)

It is a little disturbing that adding 4 lines to a file can slow down loading by 10 seconds, so I thought someone might want to look into this. (I'm guessing that there is some performance gotcha associated with defining new constructors for very common types like Number.)

@stevengj stevengj added the performance Must go faster label Jun 15, 2018
@mbauman
Copy link
Member

mbauman commented Jun 15, 2018

Won't that invalidate any code that calls T(::Any) methods? Non-deprecated ones that exist are Symbol(x…), AbstractArray(arg), and probably most importantly Dict(kv) and Tuple(itr).

@stevengj
Copy link
Member Author

Only if you call the code with a PyObject, no?

@JeffBezanson
Copy link
Member

Yes, but that also affects any highly generic call site like Dict(x::Any). Ideally we could see that even though a new method became applicable, it doesn't have any behavior that differs from the existing constructors, and so no invalidations are needed. The mechanism is just not that sophisticated yet.

Unfortunately this is just a really difficult edge case --- you're not often able to write "here's a way to construct almost anything".

@JeffBezanson
Copy link
Member

Where possible, a better approach is to adapt PyObjects to interfaces expected by existing constructors and conversions, rather than adding new constructors. For example Dict(pairs(pyobj)).

@stevengj
Copy link
Member Author

Currently I’m just defining convert methods and not constructors on 0.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

No branches or pull requests

4 participants