-
Notifications
You must be signed in to change notification settings - Fork 370
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
update precompilation statements #2718
Conversation
@nalimilan - as you can see after despecialization + hand editing the file is much smaller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
# sed -i '/categorical/di' src/other/precompile_tmp.jl # Remove CategoricalArrays uses | ||
# sed -i '/var"/d' src/other/precompile_tmp.jl # var"" is not supported on old Julia versions | ||
# sed -i '/materialize!/d' src/other/precompile_tmp.jl # Work around an inference bug | ||
# sed -i '/setindex_widen_up_to/d' src/other/precompile_tmp.jl # Not present on Julia 1.0 | ||
# sed -i '/restart_copyto_nonleaf!/d' src/other/precompile_tmp.jl # Not present on Julia 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you didn't use these at all? Any step that can limit the amount of hand editing is good to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of signatures that were generated by SnoopCompile.jl matched any of these rules 😄. Things change fast. (ah - maybe there were some var"
ones)
But I see that something is failing on 1.0 - I will check what.
OK. So we have two problems. On Julia 1.0.5 we have the following error:
which seems a bug on Julia side. The other problem is on x86 architecture where we get a problem of What I did was:
and added information on what editing steps were made for the future. What do you think? CC @timholy - both the Julia 1.0 problem and the |
The timings for joins are as follows (they uniformly improve). innerjoin0.22.7:
this PR:
outerjoin0.22.7:
this PR:
crossjoin0.22.7:
this PR:
|
Some more complex 0.22.7:
this PR:
in reshaping we do not have an improvement, but we can work on it in the future: 0.22.7
this PR
|
@nalimilan - this is where we have a problem. In split-apply-combine the first call in fast path is more expensive (but other are on par). Also fast path in split-apply combine0.22.7
this PR
normal path in split-apply-combine0.22.7
this PR
|
src/other/precompile.jl
Outdated
# * disabling precompilation on Julia older than 1.6 | ||
function precompile(all=false) | ||
VERSION >= v"1.6" || return nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's check that this works on 1.5? Better avoid regressions for people who haven't moved to 1.6 yet.
# * disabling precompilation on Julia older than 1.6 | |
function precompile(all=false) | |
VERSION >= v"1.6" || return nothing | |
# * disabling precompilation on Julia older than 1.5 | |
function precompile(all=false) | |
VERSION >= v"1.5" || return nothing |
Also, maybe the crash on 1.0.5 is due to a particular precompile
call? IIRC the materialize!
line above was there to remove a line that created issues on 1.0 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but the difference was that materialize!
was crashing the call to precompile
function. Here we have a problem that the crash happens after precompilation - during actual function call Julia compiler gets confused. I am OK to turn on 1.5 though. I will test if all works on 1.5.
Too bad. Yet I see several |
We could take the fast path only when the number of rows is large, just like we do for threads. But it's not totally obvious why the fallback should have a lower overhead, so some experimentation is needed. |
This is the reason I assume. |
Co-authored-by: Milan Bouchet-Valat <[email protected]>
If I understand the situation correctly number of rows is not a compile time constant, so the compilation will happen always no matter how many rows we have and no matter what column types we have. |
I have checked Julia 1.5.4 both single and multi-thereaded on Win10 and all was OK. However, given the problems we had I would merge this sooner than later and ask users to do a bit of testing on different platforms. |
Yeah, but maybe there's a way to prevent Julia from compiling all functions that may be called until that's actually the case? Not sure. |
It would be possible if the compiler could prove that it is impossible to have some function called. If e.g. we had What I was fighting for with despecialization is to make sure that - where possible - only one method gets compiled for each function (as before despecialization many were compiled for various variants of possible argument types). OK - I am merging this to allow for user testing. Thank you! |
Here is the impact:
0.22.7:
main:
this PR:
Of course more testing is welcome (and I will report additional results during the week).
CC @pdeffebach as this affects DataFramesMeta.jl