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

more per-module compiler options #37041

Merged
merged 1 commit into from
Aug 28, 2020
Merged

more per-module compiler options #37041

merged 1 commit into from
Aug 28, 2020

Conversation

JeffBezanson
Copy link
Sponsor Member

@JeffBezanson JeffBezanson commented Aug 14, 2020

This is an expansion of @optlevel, giving you e.g. @compiler_options compile=min optimize=0 infer=false. The first two correspond directly to the --compile and --optimize command-line options, and the infer one is new. I like the idea of basically just changing some command-line options at the module level. @options is very generic so I'd appreciate any suggestions for a better name.

@StefanKarpinski
Copy link
Sponsor Member

Maybe @compiler_options? After all, this doesn't need to be short.

@andreasnoack
Copy link
Member

I tried to use this option in all Gadfly modules to measure the effect on TtFP. I used this example, http://gadflyjl.org/stable/lib/statistics/#Gadfly.Stat.density, with the master versions of Gadfly and Compose but after setting @options compile=min optimize=0 infer=false in all modules of Gadfly I got

ERROR: LoadError: MethodError: no method matching evalmapping(::DataFrame, ::Symbol)
Closest candidates are:
  evalmapping(::Any, ::AbstractArray) at /Users/andreasnoack/.julia/dev/Gadfly/src/mapping.jl:183
  evalmapping(::Any, ::Distribution) at /Users/andreasnoack/.julia/dev/Gadfly/src/mapping.jl:185
  evalmapping(::Gadfly.MeltedData, ::Symbol) at /Users/andreasnoack/.julia/dev/Gadfly/src/mapping.jl:194
  ...
Stacktrace:
 [1] _evalmapping!(mapping::Dict{Symbol,Any}, data_source::DataFrame, data::Gadfly.Data)
   @ Gadfly ~/.julia/dev/Gadfly/src/mapping.jl:204
 [2] evalmapping!(mapping::Dict{Symbol,Any}, data_source::DataFrame, data::Gadfly.Data)
   @ Gadfly ~/.julia/dev/Gadfly/src/mapping.jl:233
 [3] plot(::DataFrame, ::Dict{Symbol,Symbol}, ::Type{Gadfly.Geom.LineGeometry}, ::Type{Gadfly.Geom.RibbonGeometry}, ::Gadfly.Guide.YLabel, ::Theme, ::Vararg{Union{Vector{Layer}, Function, Gadfly.Element, Theme, Type},N} where N)
   @ Gadfly ~/.julia/dev/Gadfly/src/Gadfly.jl:331
 [4] plot(::DataFrame, ::Type{Gadfly.Geom.LineGeometry}, ::Vararg{Union{Vector{Layer}, Function, Gadfly.Element, Theme, Type},N} where N; mapping::Base.Iterators.Pairs{Symbol,Symbol,NTuple{5,Symbol},NamedTuple{(:x, :y, :ymin, :ymax, :color),NTuple{5,Symbol}}})
   @ Gadfly ~/.julia/dev/Gadfly/src/Gadfly.jl:293
 [5] top-level scope
   @ ~/Desktop/test_gadfly.jl:7
in expression starting at /Users/andreasnoack/Desktop/test_gadfly.jl:7

@staticfloat
Copy link
Sponsor Member

Hmmm, it appears that if I use Base.Experimental.@options compile=min optimize=0 infer=false within module Foo, then Foo.__init__() does not get called after a using Foo.

@JeffBezanson
Copy link
Sponsor Member Author

Fixed.

@staticfloat
Copy link
Sponsor Member

Just ran an experiment with some WIP branches of BinaryBuilder and tall stacks of JLLs; it turns out that with this branch and the recent improvements that Mosè and I have been working on for BinaryBuilder, I can load GTK3_jll in 410ms! That's a huge improvement!

For an end-to-end test with much more than just loading JLL packages, we can step back in time a little bit and look at "Time To First Face of Victory":

using TestImages, ImageView
imshow(testimage("mandrill"))

Running that segment on latest master versus my branch drops from 13.4s to 10.0s on my machine, and the vast majority of the remaining time is no longer JLL packages, since loading the large stacks of JLL packages themselves went from 4.9s to 0.3s. This is huge, and to underscore the many pieces that have come together for this, consider that the same code takes 26s to run on Julia 1.4.2.

@bramtayl
Copy link
Contributor

Maybe it would be worth considering keeping compile options in an optional separate TOML file or something?

@JeffBezanson JeffBezanson changed the title WIP: more per-module compiler options more per-module compiler options Aug 25, 2020
@JeffBezanson
Copy link
Sponsor Member Author

Ready to go.

@andreasnoack I suspect that was the same __init__ issue; I tried the Stat.density example and it seems to work now.

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Aug 31, 2020

A.
It's frustrating, I do on my brand-new master, or should be getting the over 10x faster:

$ julia -O1 --compile=min -q
julia> @time using Zlib_jll
  0.012673 seconds (12.31 k allocations: 981.233 KiB)

vs. on defaults:

julia> @time using Zlib_jll
  0.164142 seconds (115.67 k allocations: 6.851 MiB)

I did add to the module:

if isdefined(Base, :Experimental) && isdefined(Base.Experimental, Symbol("@compiler_options"))
    @eval Base.Experimental.@compiler_options compile=min optimize=1 infer=false
end

and the "infer=false" part seems to work (that line is run, confirmed by injecting typo), if I skip only that part, I get consistently slower for the former timing. But for the latter, on defaults, the code seems to do nothing! Or at lest isn't fully effective (for JLLs). So does Jeff PR have a bug?

For the former loading.jl: _include_from_serialized: line 586 takes like half of the time, and for the latter like 80%.

B.
At the top where Jeff wrote @options is outdated, maybe update there. At least that's not my error, as that way the code fails.

@JeffBezanson
Copy link
Sponsor Member Author

I get the following times:

no options in Zlib_jll:
default command line: 0.143 sec
-O1 --compile=min on command line: 0.037 sec

adding options to Zlib_jll:
default command line: 0.121 sec
-O1 --compile=min on command line: 0.018 sec

I don't see any surprises from removing just infer=false either. This is all in line with what I'd expect.

Of course, passing -O1 on the command line will make a much bigger difference since it affects everything and not just one module.

@JKRT
Copy link

JKRT commented Nov 13, 2020

This PR has been really helpful for me!

Is there current any work in improving the granularity? For instance, rather than turning of inference altogether instead considering fewer methods.

I have been experimenting with it myself with some success

timholy added a commit that referenced this pull request Sep 25, 2021
There was an unintended collision in meaning between #42128 and #37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373
timholy added a commit that referenced this pull request Sep 28, 2021
This more explicit directive distinguishes `@compile`
(now moved to Experimental) from `@compiler_options`
(which uses the same `:compile` symbol). It eliminates
a collision between #42128 and #37041.

Fixes #42373
Closes #42379
JeffBezanson pushed a commit that referenced this pull request Oct 20, 2021
There was an unintended collision in meaning between #42128 and #37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373
JeffBezanson pushed a commit that referenced this pull request Oct 21, 2021
There was an unintended collision in meaning between #42128 and #37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373
timholy added a commit that referenced this pull request Oct 22, 2021
There was an unintended collision in meaning between #42128 and #37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373
Comment on lines +516 to +518
if ccall(:jl_get_module_infer, Cint, (Any,), method.module) == 0
return Any, nothing
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

this needs to go at the top of the method (before looking at the cache), to ensure this code is stable

Comment on lines +623 to +625
if ccall(:jl_get_module_infer, Cint, (Any,), method.module) == 0
return retrieve_code_info(mi)
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

same here. and also because this code is holding a lock, which must be released before returning

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I am not sure this code is needed here, though, or if it should go in jl_generate_fptr_impl with the other heuristics. It feels like the implementation here is instead dictating policy decisions here, and ignoring the user's specific instruction.

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
There was an unintended collision in meaning between JuliaLang#42128 and JuliaLang#37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes JuliaLang#42373
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
There was an unintended collision in meaning between JuliaLang#42128 and JuliaLang#37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes JuliaLang#42373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants