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

add "#pragma compile [true]" for opt-in to automatic compilation #12475

Closed
wants to merge 1 commit into from

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Aug 5, 2015

This PR implements automatic opt-in to compilation for modules, as discussed in #12462. In particular:

  • If the module Foo's file Foo.jl starts with #pragma compile [ true] (before any non-comment code), then require(:Foo) (hence import or using) will automatically invoke Base.compile(:Foo).
  • The default is not to compile, i.e. opt-in. Any other default would be unsafe, and Julia is a safe language by default. (You can still manually call Base.compile, however.)
  • If you explicitly put #pragma compile false at the top of the module, then Base.compile will throw an error for that module. (Since Base.compile is recursive, this will also throw an error if you attempt to compile a module that requires a non-compilable module.)

(In the future, it might be nice to implement compilation of modules that depend on non-compilable modules, but this is new functionality and should be a separate PR.)

Probably better to merge this after #12448 (.ji magic header) and #12458 (automated recompilation).

@stevengj stevengj added the compiler:precompilation Precompilation of modules label Aug 5, 2015
@stevengj stevengj force-pushed the ji_optin branch 2 times, most recently from 7692b69 to de4605c Compare August 5, 2015 20:07
@StefanKarpinski
Copy link
Member

I think that having a general #pragma syntax is kind of the bigger design issue here. cf my proposal in #7449, which was not exactly popular.

@StefanKarpinski
Copy link
Member

Specifically, I dislike the = in this syntax, since it implies that pragmas are like a real programming language, which they should certainly not be. Something simple like #pragma compile, #pragma compile true or #pragma compile false seems better – each pragma has a name and arguments; if the name is unrecognized, the pragma is ignored. Another design question is what to do if the argument to the pragma is not recognized.

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

I'm fine with removing the = and shortening compilable to compile. #pragma compile <unrecognized> should be ignored, like any unrecognized pragma.

All of the alternatives for this seem worse (tie compilation to Pkg, require double parsing or (worse) double eval, have per-module rather than the actual per-file semantics of our compiler, etcetera).

And without automated opt-in, Base.compile is very problematic from an interface perspective. (I shudder to imagine explaining to my students "use Base.compile, but only for this small set of modules, because anything else it might crash).

@quinnj
Copy link
Member

quinnj commented Aug 5, 2015

Sounds good to me.

Overall, I think we've gotten around #7449 quite well with Compat, but this seems like a compelling-enough case where we need some kind of pragma. Perhaps that's a good compat rule of thumb; use it anywhere we can, with the #pragma fallback.

@tkelman
Copy link
Contributor

tkelman commented Aug 5, 2015

My biggest dislike of this is that it's in a comment. We're now making the content of comments semantically meaningful for the first time, because we couldn't agree on the need far enough in advance to give pragmas a recognized dedicated syntax for compatibility. This can't be the permanent way we do pragmas - code generated programmatically won't have any way of representing them.

Is it really so intractable as

if VERSION >= v"0.4.0"
include("compilable.jl")
end

?

@stevengj stevengj changed the title add "#pragma compilable [=true]" for opt-in to automatic compilation add "#pragma compile [true]" for opt-in to automatic compilation Aug 5, 2015
@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

Updated to #pragma compile [true]

@ScottPJones
Copy link
Contributor

Please see my comment #12462 (comment) about adding a macro with no arguments @Compilable

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

@tkelman, your solution requires the file to be evaled, which has problems already discussed (it involves an expensive double-eval when compiling, and it means that the compiled version is not the one loaded when you do using the first time). Also Base.compile is file-based, so code that is generated but not outputted to a file is not relevant here.

ismatch(r_compilable, s) && return true
ismatch(r_ncompilable, s) && return false
else
return default
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this does the wrong thing for lines inside a block comment. Or #pragma compilable would also have to be documented as needing to come before any multi-line block comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh multiline comments, whoever implemented such an annoying feature?

Initially, we could just tend to document it as coming before #= ... =# block comments (in practice I don't think this would affect people much), but I agree that you'd probably want to relax that eventually. Fortunately, parsing comments (even with nesting) is easy, and this could be added incrementally.

@ScottPJones
Copy link
Contributor

@stevengj I don't think that's correct, same as with what I was proposing, because the parsing code can short-circuit out of parsing/evaluating the rest of the file if it wants. (I tested that in a dumb way by causing a syntax error in an included file, and I saw that the rest of the file was not parsed or evaluated.)

@stevengj
Copy link
Member Author

stevengj commented Aug 6, 2015

Closing in favor of #12491

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.

5 participants