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

Unconditional max_methods=1 #462

Merged
merged 2 commits into from
Jan 25, 2023
Merged

Unconditional max_methods=1 #462

merged 2 commits into from
Jan 25, 2023

Conversation

chriselrod
Copy link
Member

@chriselrod chriselrod commented Jan 24, 2023

@ChrisRackauckas no fear
max_methods=1 can't hurt you if you're pure of heart type stable.

@chriselrod
Copy link
Member Author

Well, it doesn't seem to help either.
PR:

julia> @time @eval function AmulB!(C,A,B)
           @turbo for n  indices((C,B),2), m  indices((C,A),1)
               Cmn = zero(eltype(C))
               for k  indices((A,B),(2,1))
                   Cmn += A[m,k] * B[k,n]
               end
               C[m,n] = Cmn
           end
       end
  0.023787 seconds (4.70 k allocations: 271.455 KiB, 76.64% compilation time)
AmulB! (generic function with 1 method)

julia> M = K = N = 71; A = rand(M,K); B = rand(K,N); C1 = A*B; C0 = zero(C1);

julia> @time @eval AmulB!(C0, A, B);
  2.113024 seconds (4.32 M allocations: 261.378 MiB, 3.93% gc time, 99.99% compilation time)

Latest release:

julia> @time @eval function AmulB!(C,A,B)
           @turbo for n  indices((C,B),2), m  indices((C,A),1)
               Cmn = zero(eltype(C))
               for k  indices((A,B),(2,1))
                   Cmn += A[m,k] * B[k,n]
               end
               C[m,n] = Cmn
           end
       end
  0.024604 seconds (4.70 k allocations: 271.307 KiB, 76.93% compilation time)
AmulB! (generic function with 1 method)

julia> M = K = N = 71; A = rand(M,K); B = rand(K,N); C1 = A*B; C0 = zero(C1);

julia> @time @eval AmulB!(C0, A, B);
  2.093600 seconds (4.30 M allocations: 260.192 MiB, 4.07% gc time, 99.99% compilation time)

Of course, there is a ton of type unstable code in LV to process expressions.

Maybe I should merge this anyway as a symbolic gesture.

@ChrisRackauckas
Copy link

I was wondering if it could effect compilation downstream though when it's being hit behind a function barrier (due to the autoswitch solvers). That's really tedious to test though.

@chriselrod
Copy link
Member Author

chriselrod commented Jan 24, 2023

Of course, there is a ton of type unstable code in LV to process expressions.

Which is why I was thinking this may help.

I was wondering if it could effect compilation downstream though when it's being hit behind a function barrier (due to the autoswitch solvers). That's really tedious to test though.

I'm going to @descend through the calls in OrdinaryDiffEq's precompile statements and see if I run into instabilities.

But I think
@max_methods 1 as a default, and then special casing specific methods that need more (if we have no other way of fixing it) is a reasonable approach.

For reference: JuliaLang/julia#44426

But I think we should also see more examples where it helps before going all out.
One of us should build Julia that defaults to max_methods=1 and see if OrdinaryDiffEq compiles any faster vs default Julia master before committing to anything like PRing max_methods=1 into the entire JuliaSIMD and SciML stacks.

@chriselrod
Copy link
Member Author

I'm going to @descend through the calls in OrdinaryDiffEq's precompile statements and see if I run into instabilities.

I run into type instabilities immediately.

@ChrisRackauckas
Copy link

There should be some due to the auto-switch, which does manual branching in order to get around the way it would dispatch, but I'm not sure it gives good precompilation.

@chriselrod
Copy link
Member Author

chriselrod commented Jan 24, 2023

Even without autoswitch

EDIT: false alarm

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Base: 87.00% // Head: 86.84% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (99afd57) compared to base (46d3333).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
- Coverage   87.00%   86.84%   -0.17%     
==========================================
  Files          38       38              
  Lines        9372     9372              
==========================================
- Hits         8154     8139      -15     
- Misses       1218     1233      +15     
Impacted Files Coverage Δ
src/LoopVectorization.jl 75.00% <ø> (ø)
src/codegen/lower_constant.jl 72.76% <0.00%> (-2.82%) ⬇️
src/reconstruct_loopset.jl 91.79% <0.00%> (-1.18%) ⬇️
src/modeling/determinestrategy.jl 94.92% <0.00%> (-0.32%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chriselrod chriselrod merged commit b4424f8 into main Jan 25, 2023
@chriselrod chriselrod deleted the maxmethods1v2 branch January 25, 2023 12:29
This pull request was closed.
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.

2 participants