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

Use -O1 optimization level #682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use -O1 optimization level #682

wants to merge 2 commits into from

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Sep 21, 2020

Extracted from #681 so that this can have some more discussion and not slow down that PR. From the other PR:

[This] commit enables a lower optimization level for the entire module in Julia 1.5+. The motivation here is that a lot of functions do not type infer, so we might as well tell the compiler to not try too hard.

@musm
Copy link
Member

musm commented Sep 21, 2020

I think Plots.jl is having to do something similar to this.

@jmert
Copy link
Contributor Author

jmert commented Oct 29, 2020

Just out of curiosity: rebased on master (307e7db), and running my timing script:

   master precompile:  2.272 ± 0.0294
   master pkg load:    0.624 ± 0.0097
   master pkg test:   47.815 ± 0.6639
optlevel1 precompile:  2.235 ± 0.0462
optlevel1 pkg load:    0.549 ± 0.0110
optlevel1 pkg test:   40.586 ± 0.4779

@musm
Copy link
Member

musm commented Oct 29, 2020

I'm fine with merging this as it does help, as long as we add some comments on it that it's likely unnecessary if we manually fix invalidations in the future.

@jmert
Copy link
Contributor Author

jmert commented Oct 29, 2020

I'm not in a rush to merge this — was just rebasing some branches and thought I'd just leave a contextual breadcrumb.

Invalidations aren't the thing that this is trying to solve — the invalidations are the same on both master and this branch:

julia> using SnoopCompileCore

julia> invs = @snoopr include("test/runtests.jl")
[ Info: Precompiling HDF5 [f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f]
HDF5 version 1.10.4
...

julia> using SnoopCompile

julia> invalidation_trees(invs)
2-element Vector{SnoopCompile.MethodInvalidations}:
 inserting datatype(::Type{foo_hdf5}) in Main at /home/justin/.julia/dev/HDF5/test/compound.jl:32 invalidated:
   mt_backedges: 1: signature Tuple{typeof(datatype), Type} triggered MethodInstance for d_create_external(::HDF5.File, ::String, ::GenericString, ::Type, ::Tuple{Int64, Int64}, ::Int64) (1 children)

 inserting names(x::Union{HDF5.Attributes, HDF5.File, HDF5.Group}) in HDF5 at deprecated.jl:70 invalidated:
   mt_backedges: 1: signature Tuple{typeof(names), Any} triggered MethodInstance for iterate(::Base.Generator{Vector{Any}, typeof(names)}, ::Int64) (3 children)
                 2: signature Tuple{typeof(names), Any} triggered MethodInstance for iterate(::Base.Generator{Vector{Any}, typeof(names)}) (4 children)
   10 mt_cache

It's just that compiler optimizations don't achieve much on poorly type-inferred results, so we can head that off by just telling the compiler to not try very hard at optimizing the generated code.

@musm
Copy link
Member

musm commented Oct 29, 2020

Oh I see, thanks for the clarification. So we have a lot of poorly type-inferred results. It might be tricky to improve the situation on that front.

@jmert
Copy link
Contributor Author

jmert commented Oct 29, 2020

Yeah, its the very dynamic nature of operations like e.g. read(::Dataset) -> ??? that is going to be the limiting case.

I'm guessing something that will also help is using the other SnoopCompile features and seeing if/where some @nospecialize, ::Any type assertions, and/or other purposeful type-widening might just head off unnecessary inference and compiled specializations.

@musm
Copy link
Member

musm commented Dec 7, 2020

For me timings have now improved, the tests timing difference is about ~ 1 s. And the differences in load and precompile are within 1/10 of a second.

@jmert
Copy link
Contributor Author

jmert commented Dec 9, 2020

This is what I get on master, a rebased version of this PR, and then a branch where I've additionally removed the deprecations file.

   master precompile:  2.378 ± 0.0274
   master pkg load:    0.562 ± 0.0123
   master pkg test:   52.051 ± 1.6150
optlevel1 precompile:  2.284 ± 0.0135
optlevel1 pkg load:    0.481 ± 0.0120
optlevel1 pkg test:   43.644 ± 0.7541
  no_deps precompile:  2.137 ± 0.0149
  no_deps pkg load:    0.482 ± 0.0092
  no_deps pkg test:   43.443 ± 0.5663

So package precompile and load times are very close now, but optlevel 1 still reduces the entire test suite time by ~8 seconds (~15% reduction from master).

@mkitti mkitti requested a review from musm May 31, 2022 21:23
Copy link
Member

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

We should do some measurements, but I find it unlikely we are doing anything compute intensive here that warrants extensive optimization. I thus recommend we merge this.

@musm
Copy link
Member

musm commented May 31, 2022

I'd also recommend testing/benchmarking

if isdefined(Base, :Experimental) && isdefined(Base.Experimental, Symbol("@max_methods"))
    @eval Base.Experimental.@max_methods 1
    @eval Base.Experimental.@optlevel 0
end

@mkitti
Copy link
Member

mkitti commented May 31, 2022

Last CI run on master, julia-actions/julia-runtest@latest took 1m 58s for ubuntu-latest Julia 1.
On this pull request, julia-actions/julia-runtest@latest took 1m 46s for ubuntu-latest Julia 1.

@mkitti
Copy link
Member

mkitti commented May 31, 2022

Looking further, it's a bit of a wash.

Platform master -O1 optimization
ubuntu-latest 1m 58 s 1m 46 s
macOS 1m 55s 2m 16s
windows x64 2m 15s 2m 2s
windows x86 2m 33s 2m 43 s

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.

3 participants