move builtin generation logic to precompilation time #139
Conversation
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
=====================================
Coverage 0% 0%
=====================================
Files 14 14
Lines 1643 1645 +2
=====================================
- Misses 1643 1645 +2
Continue to review full report at Codecov.
|
src/JuliaInterpreter.jl
Outdated
| @@ -20,12 +20,22 @@ module CompiledCalls | |||
| # This module is for handling intrinsics that must be compiled (llvmcall) | |||
| end | |||
|
|
|||
| const BUILTIN_FILE = joinpath(@__DIR__, "builtins-julia$(Int(VERSION.major)).$(Int(VERSION.minor)).jl") | |||
|
|
|||
| if !isfile(BUILTIN_FILE) | |||
There was a problem hiding this comment.
Should we also check Base.stale_cachefile("generate_builtins.jl", cachefile)? I'm not sure I'm thinking about the logic of this properly, but it seems to make sense to rebuild it if the generator changes.
If possible, it would be lovely to preserve the ability to edit the "builtins-" file for the purposes of debugging. If I'm thinking about it correctly, it seems that checking the mtime on "generate_builtins.jl" but not "builtins-" will preserve that capability.
There was a problem hiding this comment.
Storing the mtime of generate_builtins.jl and rerun the build script if it changed makes sense.
If possible, it would be lovely to preserve the ability to edit the "builtins-" file for the purposes of debugging.
The changes here shouldn't affect that?
There was a problem hiding this comment.
Right, that's what I'm expecting too. But sometimes the chain of dependencies makes my head hurt.
There was a problem hiding this comment.
I don't think you have to store the mtime, do you? That comparison should be run for you by stale_cachefile.
There was a problem hiding this comment.
Maybe another approach: put this logic inside a ccall(:jl_generating_output, Cint, ()) == 1 and add the generate_builtins.jl as an include_dependency? If you wanted to get fancy, you could even run it in a separate process, and then generate_builtins.jl would not be part of the cachefile.
There was a problem hiding this comment.
Something like this then?
KristofferC
force-pushed
the
kc/build
branch
2 times, most recently
from
5f8cfd8
to
eac8c78
Compare
src/JuliaInterpreter.jl
Outdated
| @@ -20,12 +20,21 @@ module CompiledCalls | |||
| # This module is for handling intrinsics that must be compiled (llvmcall) | |||
| end | |||
|
|
|||
| const BUILTIN_FILE = joinpath(@__DIR__, "builtins-julia$(Int(VERSION.major)).$(Int(VERSION.minor)).jl") | |||
|
|
|||
| if ccall(:jl_generating_output, Cint, ()) == 1 | |||
There was a problem hiding this comment.
Is this check really needed?
|
Hm, I think we'll need a fallback if someone runs Julia with |
|
Yeah, that was my point with #139 (comment). Should just be able to remove that. |
|
Hm but then the builtin file would be regenerated every time the module is loaded, which is a safer but more annoying default, no? |
|
Yes, every time it is loaded when running with |
Co-Authored-By: KristofferC <kristoffer.carlsson@chalmers.se>
The current build system has the problem that if someone adds JuliaInterpreter on two julia versions, the second add will not build JuliaInterpreter (since it is already downloaded and assumed built). This will then give an error when loading it that it misses the file.
Instead, just run this on precompilation time if we cant find the file.
The negative is that we need to load the code for generating the builtin file every time we load the package but from my measurements it is within noise.