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

__precompile__(isprecompilable) function for automated opt-in to module precompilation #12491

Merged
merged 1 commit into from
Aug 7, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Aug 6, 2015

This addresses #12462 by adding a new function __precompile__(isprecompilable::Bool=true):

  • If a file calls __precompile__() (typically at the top of the file before the module statement), then it will be automatically precompiled when it is imported for the first time (and thereafter will be automatically recompiled thanks to automatic recompilation of stale cache files #12458). Technically, __precompile__(true) throws an exception if the file is included in a non-precompile Julia process.
  • If a file calls __precompile__(false), then it will throw an exception during precompilation. This way, if a module is known to be unsafe to precompile, you can prevent the user from precompiling it accidentally (e.g. by importing it into another module that is being compiled).
  • Backwards compatibility with 0.3 is easy: just use VERSION >= v"0.4-" && __precompile__(). We should also put __precompile__ into Compat for completeness, but it is nice to be able to put __precompile__() before the module ... end in order to prevent the module from being parsed or eval-ed (even partially) when we are detecting precompilability.
  • Base.compile(mod) (which should now be rarely needed by users) is renamed to Base.compilecache(mod) to better describe its function.

Thanks to @ScottPJones for his suggestion. This turned out to be much cleaner than the #pragma approach in #12475, with no loss of functionality.

@timholy
Copy link
Sponsor Member

timholy commented Aug 6, 2015

Very nice resolution---good suggestion @ScottPJones!

@ScottPJones
Copy link
Contributor

😀 Finally I've managed to do something right around here! Thanks! (maybe this is my start on working off my immense bikeshedding debt!)

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

Of, course, this wouldn't be a proper Julia PR if we don't immediately commence a huge bikeshed over spelling. Options:

  • Verb: compilethis(true), compileme(true), compile_me(true), compilefile(true), compile(true) [not to be confused with compile(module)], __compile__(true), ...
  • Adjective: compilable(true)
  • Noun: compiler(true)

Plus many more-verbose options.

@mbauman
Copy link
Sponsor Member

mbauman commented Aug 6, 2015

Happy to start the ball rolling. What about something like pragma(:compile, true)? I suppose that might get confusing if we ever get lexical pragmas…

Maybe just a more explicit compilefile (this is ambiguous between the module or the file).

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@mbauman, I avoided pragma because this isn't really a pragma directive in the usual sense: it is an ordinary Julia function, executed in the ordinary way.

I avoided compilefile(arg) because it sounds like it is going to compile arg, but I guess you could say the same of compilethis(arg)...it just seemed like compilefile more explicitly implied that its argument is a file, but that's only a vague impression. I guess we also could do compilethisfile(true) or compilecurrentfile(true), but if we go in that direction I'd prefer the shorter compilable(true).

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

(I chose a verb over an adjective because compilable(true) sounds like it is just a declaration of information, whereas it is actually a requirement that the file only be included in compiled form. But I don't have strong feelings about this; it's nothing that documentation can't fix, and the user will probably have to read the documentation on this function anyway.)

@ssfrr
Copy link
Contributor

ssfrr commented Aug 6, 2015

compile(@__FILE__) ?

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@ssfrr, no, because that incorrectly implies that it will compile an arbitrary file argument, whereas this only forces the compilation of the file(s) where it is executed (and technically only as a side effect of import; the compilethis function itself doesn't perform the compilation). Plus, we already have Base.compile(module) to actually perform the compilation.

@davidagold
Copy link
Contributor

compileme()?

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@davidagold, yes, compileme works for me (though I'm tempted to pronounce it "kom - pi - LEEM" and interpret as some obscure CS version of "meme"); we could also spell it compile_me(true).

@StefanKarpinski
Copy link
Sponsor Member

__compile__(true)?

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

👍 for compile_me

@davidagold
Copy link
Contributor

@stevengj Me too. The underscore makes quite a difference in readability.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@StefanKarpinski, yes, __compile__ is the "scare off the casual programmer" route. Which might be a good idea, since precompilation is really only for people who understand the words "side effect", "pointer", and "runtime". (Only hard-core programmers use double underscores!)

@mbauman
Copy link
Sponsor Member

mbauman commented Aug 6, 2015

What about just expressing that fact? assert(iscompiling()) or assert(!iscompiling()). This won't work as is, since you detect if the error from include was because of this… and I don't really see a way around that. But it's a spitball.

+:100: for the functionality (and all your work this week), and I think the __compile__ form is pretty good if we can't find anything better.

@ScottPJones
Copy link
Contributor

Argh, I hate to bring this up, but I don't think "compile" is really the right word here at all.
The code is going to be compiled, or it can't run.
The question is rather, is it cacheable.
What about cachecode(true)?

@ScottPJones
Copy link
Contributor

Also, sorry to contribute more to bikeshedding, but "me" seems a bit too cute. The code is not a person.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@ScottPJones, I agree that technically what is happening is that it is caching the compiled version. But I think that in practice most people will think of it in shorthand as "compiling" the code.

Anyway DOUBLE UNDERSCORES will warn the reader that something funky is going on.

@ScottPJones
Copy link
Contributor

I think that they already know it is compiling the code, and in fact, __compile__(true) seems backwards, because what it means is you want to use that cache instead of always compiling the code.

@ScottPJones
Copy link
Contributor

__cache_compiled_code__(true) I think would be totally clear, what do you all think?

@quinnj
Copy link
Member

quinnj commented Aug 6, 2015

+1 to __compile__(true)

@stevengj stevengj changed the title compilethis(iscompilable) function for automated opt-in to module compilation __compile__(iscompilable) function for automated opt-in to module compilation Aug 6, 2015
@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

Someone needs to bump appveyor.

@StefanKarpinski
Copy link
Sponsor Member

__compile__(true) and __compile__(false) seem to send the right message, and be the right amount of awkward. The underscores say "this is some scary internal stuff, don't mess with it lightly".

@StefanKarpinski
Copy link
Sponsor Member

Someone needs to bump appveyor.

I spent about ten minutes on trying to get this build to go again, but the AppVeyor interface has defeated me. It refuses to restart the build, telling me to contact support. The support contact page requires you authenticate separately from the main app, but only supports password and Google auth, while my account is setup to authenticate with GitHub – which isn't an option. I also can't figure out where in the interface one goes to give others admin rights to the repo or how to create an email / password login (changing the password requires entering an old password, which I don't have).

@ScottPJones
Copy link
Contributor

Of course, I also think that Base.compile should have been Base.precompile, the code will be compiled any way, just earlier instead of JIT compilation.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@StefanKarpinski, this is appveyor/ci#353, and @tkelman was able to force a new build number in some other PRs IIRC.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

I'm fine with Base.precompile and __precompile__.

@ScottPJones
Copy link
Contributor

👍 to @stevengj's suggestion, succinct and accurate!

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

(Of course, there already is a precompile(f, argtuple) function, which is distinct from Base.compile. But the latter can probably stay as-is ... it is accurate, it is compiling a module, and anyway it's not exported and there isn't as much need for it given __precompile__. Or maybe call it Base.compilecache?)

@ihnorton
Copy link
Member

ihnorton commented Aug 6, 2015

(nvm, I see the error message. maybe try a force push now that the build number has been bumped a few times?)

@FeodorFitsner
Copy link

I've reset that stuck build. When the build version collision is happening we are seeing database timeouts. We are working to resolve this issue.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

I'll do a force push in a minute.

@stevengj stevengj changed the title __compile__(iscompilable) function for automated opt-in to module compilation __precompile__(isprecompilable) function for automated opt-in to module precompilation Aug 6, 2015
@ScottPJones
Copy link
Contributor

At the risk of getting pelted with rotten virtual 🍅s, what about the following minor changes:
rename Base.compile as Base.__precompile__, which does make it's name better match it's function (and stand out as "here be dragons" with the __s, and make the exported function that will be everywhere to be __precompilable__(isprecompilable). I think it makes the action of precompiling something more distinct from asserting the trait that a file only contains precompilable modules.

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

See my above comment on adjectives. I'm not too concerned about Base.compilecache or whatever, since calling it should be almost never needed. We might even remove it from the manual.

@ScottPJones
Copy link
Contributor

Maybe assert_precompilable(isprecompilable) then? since your objection to the adjective seemed to be

whereas it is actually a requirement that the file only be included in compiled form.

@stevengj
Copy link
Member Author

stevengj commented Aug 7, 2015

I don't hear a clamor of support for a more verbose spelling.

@StefanKarpinski
Copy link
Sponsor Member

My vote: go with the current version.

On Aug 6, 2015, at 9:35 PM, Steven G. Johnson notifications@github.com wrote:

I don't hear a clamor of support for a more verbose spelling.


Reply to this email directly or view it on GitHub.

@IainNZ
Copy link
Member

IainNZ commented Aug 7, 2015

Smack that big ol' green button @stevengj

stevengj added a commit that referenced this pull request Aug 7, 2015
__precompile__(isprecompilable) function for automated opt-in to module precompilation
@stevengj stevengj merged commit d9f7699 into JuliaLang:master Aug 7, 2015
@stevengj stevengj deleted the ji_optin2 branch August 7, 2015 02:52
@ScottPJones
Copy link
Contributor

Great job, @stevengj! __precompile__ is certainly fine, I just am a perfectionist, and also want to make sure that all possibilities get discussed, which they have been here.

@tkelman
Copy link
Contributor

tkelman commented Aug 7, 2015

👍

I think brand new functions need to have signatures manually added to the RST docs at the moment for ./julia doc/genstdlib.jl to properly populate the docstring content.

@timholy
Copy link
Sponsor Member

timholy commented Aug 7, 2015

One small problem I foresee: because compilation is slower than loading the package if you only plan to do it once, developers actively working on a package are likely to (1) delete the *.ji file, and (2) comment out __precompile__. We will probably get pushes to packages that forget to remove the comment.

Seems like a small price 😄 and not very different from the other "oops"es you can make (I'm surprised I don't have more @show commits out there...), but I thought I'd mention it.

@stevengj
Copy link
Member Author

stevengj commented Aug 7, 2015

@timholy, it would be trivial to add a "development" global flag that disables __precompile__(true) if this is a concern. The only question is what the UI looks like. A command-line flag like julia --dev or julia --no-precompile, an environment variable, a global variable set in .juliarc, ...?

@timholy
Copy link
Sponsor Member

timholy commented Aug 7, 2015

I'd say let's first see how bad a problem it is.

@stevengj
Copy link
Member Author

stevengj commented Aug 7, 2015

Also, realize that if you are working on a module, only that module itself will be recompiled when you change the module's files — imported modules (assuming they haven't changed) are not recompiled. So the recompile is often considerably faster than the initial compile step.

@timholy
Copy link
Sponsor Member

timholy commented Aug 7, 2015

Oh, definitely, and the main reason I wanted precompilation so much 😄. But I'm a greedy bastard, and have already discovered I can make life just a little better by temporarily disabling it for the specific package I happen to be hacking on (at least until I finish making all my stupid mistakes).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 8, 2015

One small problem I foresee: because compilation is slower than loading the package if you only plan to do it once, developers actively working on a package are likely to (1) delete the *.ji file, and (2) comment out precompile. We will probably get pushes to packages that forget to remove the comment.

I was concerned about this too originally, but found the impact to be less than I expected when I was just working on Gtk.jl and ZMQ.jl. Having the cache always up-to-date, without needing to actively think about it, seems like such a huge improvement. I believe the developer time penalty is only about 25% (cache-load time seems to be about 10% of the load time, but the dependencies need to be loaded in both processes), on average, to build a new cache for the module – time that I felt was now being saved whenever I moved on to the usages of the module (e.g. today it was for IJulia).

As a future optimization, if we can implement file-system locking, then we could turn on parallel precompilation.

I think that while it would be somewhat faster to have it turned off for rapid iteration development, like the JIT, it shouldn't end up mattering enough to warrant much attention.

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.