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

loading: make __precompile__ the default #26991

Merged
merged 1 commit into from
Jul 31, 2018
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 5, 2018

This is nice, because it simplifies the require code. That should also make error handling more understandable and predictable. And the problems resulting from non-precompile-declaring code is used from __precompile__ code simply won't exist anymore.

fix #26282

@ararslan ararslan added the compiler:precompilation Precompilation of modules label May 5, 2018
@vchuravy vchuravy added the needs news A NEWS entry is required for this change label May 5, 2018
@StefanKarpinski
Copy link
Member

Bump?

@KristofferC
Copy link
Member

There should be a warning that people aren't allowed to write too big numbers in their code.

@StefanKarpinski
Copy link
Member

Or maybe just fix that issue?

@vtjnash vtjnash added the triage This should be discussed on a triage call label May 9, 2018
@stevengj
Copy link
Member

stevengj commented May 10, 2018

So this makes things less reliable and slower for new users (whose first modules are typically small and frequently reloaded as they develop), while not affecting load times for large packages (which inevitably turn precompilation on when they begin to mature). Great?

But I agree that this simplifies the situation for precompiled modules loading modules lacking a __precompile__ statement.

base/loading.jl Outdated
throw(PrecompilableError(isprecompilable))
@noinline function __precompile__(isprecompilable::Bool=false)
if isprecompilable
depwarn("__precompile__() is now the default", :__precompile__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deprecation seems painful: it will require an explicit VERSION check for code that wants to continue to be precompiled in 0.6 without the warning, since __precompile__() lives outside the module (before Compat is loaded).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, such version checks were the norm when precompilation was introduced. So instead of VERSION >= v"0.4.0-something" && __precompile__() it would be a check against 0.7. Not saying it's an ideal situation, just that it isn't without precedent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why, in general, changing the default value for something should require changes when the desired value is explicitly chosen by the package developer.

This required every package that didn't use __precompile__(false) to make additional changes for 0.7-rc2, while I think it should have just been limited to those packages that didn't have an explicit __precompile__()/__precompile__(true)/__precompile__(false) statement. Alternatively, automated PRs could have been opened against all packages to change this syntax.

Just my two cents.

@JeffBezanson
Copy link
Member

I'd be much more comfortable with this if the BigInt issue can be fixed first.

@JeffBezanson
Copy link
Member

It doesn't feel like this decision needs to block 0.7 alpha. Moving to 1.0.

@JeffBezanson JeffBezanson added this to the 1.0 milestone May 22, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label May 22, 2018
@vtjnash vtjnash force-pushed the jn/precompile-default branch 2 times, most recently from 4f56ec4 to 72f9e67 Compare July 20, 2018 20:21
@vtjnash vtjnash added triage This should be discussed on a triage call and removed needs news A NEWS entry is required for this change labels Jul 20, 2018
@vtjnash
Copy link
Member Author

vtjnash commented Jul 24, 2018

Hm, seems like this is trigger an issue with the Pkg tests that has snuck in since I last rebased this. There appears to be a minor misunderstanding in the way that Pkg is attempting to use Base.compilecache, based on the current limitations of that cache. (Briefly, it is checking out a package, but some statements are perhaps misordered, so it's not checking whether the result was successful. This triggers a slight interaction there whereby it can now trigger the precompile code, revealing that the result is of this uncertain consistency in Pkg state.) Until Base is fixed, we perhaps should disable the automated calls to the compilation method if there are any modules currently loaded, as Pkg is currently overriding the logic in Base that would normally do that (and is currently therefore likely wasting time, in this case, doing lots of precompile work that is not useful).

@StefanKarpinski
Copy link
Member

It seems like you're uniquely positioned to understand and fix that breakage. Let me know if there's any assistance I can provide.

@StefanKarpinski StefanKarpinski modified the milestones: 1.0, 0.7 Jul 26, 2018
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Jul 26, 2018
@ararslan ararslan mentioned this pull request Jul 26, 2018
13 tasks
@ararslan
Copy link
Member

From triage: @JeffBezanson is going to look into addressing the BigInt literal issue then this will be revisited. It's expected to be in 0.7-rc1.

@ararslan
Copy link
Member

Rebased to fix merge conflicts.

@JeffBezanson
Copy link
Member

Bump. This is unblocked now. Would be reeeeeally nice to get this in today if it's in 0.7.

@ararslan
Copy link
Member

I think the outstanding issue is the Pkg thing Jameson mentioned. @vtjnash, do you know what needs to happen to fix that?

@ViralBShah
Copy link
Member

@vtjnash Is this good to merge?

@timholy
Copy link
Member

timholy commented Aug 1, 2018

xref #28384

tkoolen added a commit to tkoolen/LoopThrottle.jl that referenced this pull request Aug 2, 2018
tkoolen added a commit to tkoolen/TypeSortedCollections.jl that referenced this pull request Aug 2, 2018
cstjean referenced this pull request in FluxML/MacroTools.jl Aug 15, 2018
@mdashti
Copy link

mdashti commented Aug 15, 2018

What's the best way to stay compatible with 0.6 in our packages, as having __precompile__(true) is an error in Julia 1.0? A related discussion here: FluxML/MacroTools.jl@18448ba#r30081444

@KristofferC
Copy link
Member

Conditionally execute it based on VERSION.

@tkoolen
Copy link
Contributor

tkoolen commented Aug 15, 2018

I've used VERSION < v"0.7.0-beta2.199" && __precompile__(). But it's really only 0.7 that complains when you have __precompile__(true), right? (after #28459)

@StefanKarpinski
Copy link
Member

0.7 is not supposed to complain about that anymore either unless that change didn't get merged.

@tkoolen
Copy link
Contributor

tkoolen commented Aug 15, 2018

Ah yes, I guess it was just the 0.7 release candidates.

epatters added a commit to AlgebraicJulia/Catlab.jl that referenced this pull request Aug 31, 2018
epatters added a commit to epatters/Serd.jl that referenced this pull request Aug 31, 2018
epatters added a commit to epatters/SPARQL.jl that referenced this pull request Aug 31, 2018
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
matteorossini added a commit to Electa-Git/FlexPlan.jl that referenced this pull request Nov 3, 2022
Julia packages are precompilable by default since 2018:
<JuliaLang/julia#26991>.

See also: `?__precompile__`.
matteorossini added a commit to Electa-Git/PowerModelsMCDC.jl that referenced this pull request Jun 6, 2023
- Remove unnecessary `__precompile__` statement. Julia packages are precompilable by default since 2018: <JuliaLang/julia#26991>.
- Update `import` statements. The `import ... as` syntax has been introduced in Julia 1.6.
- Update comments.
Keno pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make __precompile__(true) the default