-
Notifications
You must be signed in to change notification settings - Fork 16
dont @inline large constants #114
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 87.65% 87.70% +0.04%
==========================================
Files 9 9
Lines 2309 2309
==========================================
+ Hits 2024 2025 +1
+ Misses 285 284 -1
Continue to review full report at Codecov.
|
|
Thanks; yeah, it's been on my priority list to circle back on precompilation. I think, like this PR, we just need to take things piece-by-piece and figure out the biggest wins while not regressing anything. |
|
Yeah. I had some other pieces to add like a type stable |
|
What do you mean by type-stable Options? It isn't parameterized (anymore). Changing the types of some of hte fields? |
|
A lot of the fields are That's what I mean by non-linearities, sometimes type complexity has a large effect, sometimes none. I don't understand why. But it seems consistent from doing this a few times, that when you have high TTFX, replacing |
Can you please elaborate on this? |
|
I'm not sure of the cause. But initially removing unions gave a 30% or so improvement. Parsers.jl had massive compilation overhead from those inlined constants, and removing it fixed times a lot. But that meant removing the union no longer made much difference. My theory is that there was some combinatorial effect of type instability and other workloads on the compiler when it's under pressure. But that's just a gut feeling, I don't know what's actually happening internally. |
|
Can you give a code example of what you mean by unions, though? E.g., in method signatures? foo(::Union{A,B})or union-splitting or in types? |
|
I've ran into an issue a while back with unions in the past that was catastrophic in terms of precompilation: CliMA/RRTMGP.jl#352. |
|
I mean specifically |
|
Ah, yeah, we pretty consistently use concrete types. |
BIGFLOATEXP1andBIGEXP10are inlined and constant propagated everywhere, and this has a large compilation overhead. Just removing all@inlineon_scalemethods gets the whole package precompilation down to 8 seconds!!These benchmarks are using the reverted precompilation PR on main and this branch.
main:
This branch:
Should solve #113