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

Compilation time regression when extending a Base method. #20780

Closed
KristofferC opened this issue Feb 24, 2017 · 6 comments · Fixed by #20792
Closed

Compilation time regression when extending a Base method. #20780

KristofferC opened this issue Feb 24, 2017 · 6 comments · Fixed by #20792

Comments

@KristofferC
Copy link
Member

KristofferC commented Feb 24, 2017

One of my packages (https://github.com/KristofferC/Crayons.jl) went from 0.02 seconds load time after precompilation to ~ 1 seconds from 0.5 to 0.6. I was able to reduce it to the following:

type Crayon end

begin
tic()
Base.print_with_color(crayon::Crayon, io::IO, msg::AbstractString...) =
    Base.with_output_color(print, crayon, io, msg...)
toc()
end

On 0.5:

elapsed time: 0.015108936 seconds

On 0.6:

elapsed time: 1.109898606 seconds

Removing that method, load time of package goes from 0.02s to 0.04s from 0.5 to 0.6.

@StefanKarpinski
Copy link
Member

I'm guessing that's the #265 tax.

@KristofferC
Copy link
Member Author

KristofferC commented Feb 24, 2017

I don't see how this is relevant to #265? I am just adding a new method. What is different about this than extending any other method? In my view, nothing needs to be recompiled.

@yuyichao
Copy link
Contributor

It's clear whether it's #265 or not but the point of the fix is that you only pay for it at method definition time (including figuring out what should trigger the recompilation during type inference) and not the runtime. It at least does spend most of the time in type inference.

@KristofferC
Copy link
Member Author

I still think adding +1 second to a package load time (that's not even removed with precompilation) for a one line function that only extends a base method for a new type is unacceptable.

@vtjnash
Copy link
Member

vtjnash commented Feb 24, 2017

Oh, this is a bit confusing, since the time cost is in toc, not the method. There's definitely something wrong here:

julia> begin
       t0 = time()
       Base.print_with_color(crayon::Crayon, io::IO, msg::AbstractString) =
           nothing
       t1 = time()
       tic()
       t2 = time()
       toc()
       t3 = time()
       @show diff([t0, t1, t2, t3])
       end;
elapsed time: 0.048757574 seconds
diff([t0, t1, t2, t3]) = [0.00294709, 1.73136, 0.172295]

I don't see any reason that tic should have a back-edge from print_with_color.

@KristofferC
Copy link
Member Author

It exactly the same timings when I had it in my package that did not use tic, toc. Maybe the print_with_color has a bunch of "spurious back edges" to other methods...

vtjnash added a commit that referenced this issue Feb 24, 2017
This would be nicer if we had function types,
but this tries to at least hit a couple of the high points

fix #20780
vtjnash added a commit that referenced this issue Feb 24, 2017
This would be nicer if we had function types,
but this tries to at least hit a couple of the high points

fix #20780
vtjnash added a commit that referenced this issue Mar 1, 2017
This would be nicer if we had function types,
but this tries to at least hit a couple of the high points

fix #20780
vtjnash added a commit that referenced this issue Mar 2, 2017
This would be nicer if we had function types,
but this tries to at least hit a couple of the high points

fix #20780
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants