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

Putting precompile inside a version check eliminates the benefits #37509

Closed
timholy opened this issue Sep 10, 2020 · 15 comments
Closed

Putting precompile inside a version check eliminates the benefits #37509

timholy opened this issue Sep 10, 2020 · 15 comments

Comments

@timholy
Copy link
Member

timholy commented Sep 10, 2020

On 1.6, if you add this diff to Gaston.jl:

diff --git a/src/Gaston.jl b/src/Gaston.jl
index 2731d24..c7ccb7b 100644
--- a/src/Gaston.jl
+++ b/src/Gaston.jl
@@ -91,4 +91,7 @@ function __init__()
     return nothing
 end
 
+@assert precompile(plot, (UnitRange{Int},))
+@assert precompile(display, (Figure,))
+
 end

then you shave about 0.4s off TTFP (note juliamns is an alias for julia-master --startup-file=no):

tim@diva:~/.julia/dev/Gaston$ juliamns -q -e 'using Gaston'
tim@diva:~/.julia/dev/Gaston$ juliamns -q
julia> @time (using Gaston; display(plot(1:10)))
  1.617487 seconds (3.32 M allocations: 230.368 MiB, 2.88% gc time)

julia> 
tim@diva:~/.julia/dev/Gaston$ git stash
Saved working directory and index state WIP on master: eed6704 Update stable documentation link
tim@diva:~/.julia/dev/Gaston$ juliamns -q -e 'using Gaston'
tim@diva:~/.julia/dev/Gaston$ juliamns -q
julia> @time (using Gaston; display(plot(1:10)))
  2.123158 seconds (6.28 M allocations: 399.833 MiB, 4.39% gc time)

But now put those precompile statements inside a conditional (to circumvent #29859):

diff --git a/src/Gaston.jl b/src/Gaston.jl
index 2731d24..457a463 100644
--- a/src/Gaston.jl
+++ b/src/Gaston.jl
@@ -91,4 +91,9 @@ function __init__()
     return nothing
 end
 
+if VERSION >= v"1.4.2"
+    @assert precompile(plot, (UnitRange{Int},))
+    @assert precompile(display, (Figure,))
+end
+
 end

and you get no benefit:

tim@diva:~/.julia/dev/Gaston$ juliamns -q -e 'using Gaston'
tim@diva:~/.julia/dev/Gaston$ juliamns -q
julia> @time (using Gaston; display(plot(1:10)))
  2.126315 seconds (6.28 M allocations: 399.802 MiB, 4.27% gc time)

julia> VERSION >= v"1.4.2"
true
@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

And it really seems to be the version-check: this diff

diff --git a/src/Gaston.jl b/src/Gaston.jl
index 2731d24..4dc30fe 100644
--- a/src/Gaston.jl
+++ b/src/Gaston.jl
@@ -91,4 +91,12 @@ function __init__()
     return nothing
 end
 
+function _precompile_()
+    ccall(:jl_generating_output, Cint, ()) == 1 || return nothing
+    # VERSION >= v"1.4.2" || return nothing
+    @assert precompile(plot, (UnitRange{Int},))
+    @assert precompile(display, (Figure,))
+end
+_precompile_()
+
 end

works fine until you uncomment the VERSION check. It fails to deliver a benefit even if you put it in @static.

@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

CC @daschw @aminya

@KristofferC
Copy link
Member

KristofferC commented Sep 10, 2020

While precompiling, VERSION is v"1.0.2" for me... Put a @show VERSION at toplevel... Wat?

@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

😮 I get the same version.

@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

@KristofferC
Copy link
Member

KristofferC commented Sep 10, 2020

It is set at the top of the file!!!!

https://github.com/mbaz/Gaston.jl/blob/eed6704ac5faefb5e178aa7d0fa4376705a7e11e/src/Gaston.jl#L20

@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

Oh my god that is funny...took me like a minute to stop laughing. How could I not have thought to check that?

@KristofferC
Copy link
Member

I would be lying if I said I didn't re-build julia a couple of times with various @show VERSION in version.jl, grepping through the julia source code after 1.0.2 etc...

@mbaz
Copy link
Contributor

mbaz commented Sep 10, 2020

LOL! Seriously, sorry for causing such confusion guys! I'll make sure to change that variable name in the next release.

@yuyichao
Copy link
Contributor

That is a totally reasonable variable name to use as long as it's not exported. You just need to use Base.VERSION within your package.

@KristofferC
Copy link
Member

Yes, I think we all know. But apparently it is quite easy to mess up while doing so.

@yuyichao
Copy link
Contributor

yuyichao commented Sep 10, 2020

I'm just recommand against breaking the package API because of this.

@timholy
Copy link
Member Author

timholy commented Sep 10, 2020

Either way, no worries @mbaz: as of today, 1.0.2 is my new favorite number!

@aminya
Copy link

aminya commented Sep 10, 2020

I used @static checks in SnoopCompileBot. That should be different.

@KristofferC
Copy link
Member

No, it was just a namespace problem.

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

No branches or pull requests

5 participants