slightly improve type instability and add a precompile statement #5
Conversation
src/VersionParsing.jl
Outdated
| ccall(:jl_generating_output, Cint, ()) == 1 || return nothing | ||
| precompile(Tuple{typeof(VersionParsing.vparse), String}) | ||
| end | ||
| _precompile_() |
There was a problem hiding this comment.
What's the advantage of this over calling precompile(...) unconditionally?
There was a problem hiding this comment.
I've just seen everyone do it like this so it only runs when the package is precompiling (and not when it is loaded with e.g. --compiled-modules=no). But I could just do it like that if you prefer.
There was a problem hiding this comment.
ccall'ing an undocumented internal function seems like a weird idiom to encourage.
Why isn't precompile just a no-op if --compiled-modules=no?
There was a problem hiding this comment.
I don't know. How do you want to proceed here?
There was a problem hiding this comment.
Why isn't precompile just a no-op if --compiled-modules=no?
Because a few functions must be precompiled before you call them. A good example is anything that switches typeinference itself to a different implementation.
There was a problem hiding this comment.
Because a few functions must be precompiled before you call them. A good example is anything that switches typeinference itself to a different implementation.
It seems like those weird functions should be the ones doing undocumented internal ccalls, rather than forcing this idiom on everyone else…
There was a problem hiding this comment.
precompile literally means "compile now without calling it." Technically, it bears no relation to --compiled-modules=no which has to do with whether you load code from *.ji files.
But if you hate this we can just delete it. There is essentially no cost and this discussion has already grown more expensive than that.
There was a problem hiding this comment.
Removed the ccall.
src/VersionParsing.jl
Outdated
| ccall(:jl_generating_output, Cint, ()) == 1 || return nothing | ||
| precompile(Tuple{typeof(VersionParsing.vparse), String}) | ||
| end | ||
| _precompile_() |
There was a problem hiding this comment.
But you might want to do this only for Julia 1.4.2 and higher. There are occasional segfaults on earlier versions.
|
Bump can this be merged? This is the standard way of doing it in every single package that provides precompile signatures. I think having this be consistent with the rest of the ecosystem is worth it over hacking in some other workaround that looks more "official". |
|
Friendly bump. |
3 participants
This package showed up in a profile trace I did when some packages where loaded.
Before:
After: