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
add precompile runs for xparse and xparse2 #108
Conversation
Added more precompile for This shaves multiple seconds off TTFX for packages depending on JSON.jl, like Blink.jl, Interact.jl, etc. But precompile in JSON.jl still has an effect although its mostly precompiling Parsers.jl JuliaIO/JSON.jl#337 So there could be more to add here. Probably this PR should be quite thorough. Parsers.jl has 2200 dependents, so I'm guessing it's probably responsible for a significant fraction of TTFX for the whole julia ecosystem. We could also rewrite some of the long unstable methods that are causing this compile time. The abstract typed fields of the objects here may contribute. |
55a50de
to
ed2540a
Compare
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
==========================================
- Coverage 87.57% 86.77% -0.80%
==========================================
Files 9 9
Lines 2309 2321 +12
==========================================
- Hits 2022 2014 -8
- Misses 287 307 +20
Continue to review full report at Codecov.
|
So we need a test that calls the function precompile() to make codecov happy ... |
we could also just accept the code coverage regression... |
Maybe it will look better if I add it. It is making sure all those types at least run... Edit: added |
5cbe60d
to
9595a55
Compare
src/precompile.jl
Outdated
pos = 1 | ||
val = "a" | ||
len = length(val) | ||
for T in (Char, String), buf in (codeunits(val), Vector(codeunits(val))) |
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.
should also InlineStrings.jl precompilation be included here?
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.
I wouldn't think they could be included here since they're in a separate package (InlineStrings.jl); but we could do similar precompilation over there.
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.
The dependency goes the other way. Should we add a similar precompile block over there?
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.
I don't actually get much improvement with precompilation in InlineStrings.jl. Precompiling for all types only takes half a second there.
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.
What do you mean by "Precompiling for all types only takes half a second there"? Precompiling all inlinestring types? I wouldn't be too surprised by that since the heavy lifting would be handled by precompiling String
in the Parsers.jl package.
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, all the InlineString types. Without String
in Parsers.jl precompilation adds under a second to InlineStrings.jl precompilation.
It seems there is something about the numerical methods in Parsers.jl that takes a lot longer.
This should be ready to go. Can we change the actions settings here so that CI runs for anyone except first-time users? |
if !(T === String) | ||
Parsers.xparse(T, buf, pos, len, options, T) | ||
end | ||
Parsers.xparse(T, buf, pos, len, options, Any) |
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.
why is this method important to precompile? is it because it is used in CSV (as _parseany
)?
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.
Yep for _parseany
.
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.
Thanks @rafaqz!
I think this might have broken CSV.
|
The |
pos = 1 | ||
val = "123" | ||
len = length(val) | ||
for T in (String, Int32, Int64, Float64, BigFloat, Dates.Date, Dates.DateTime, Bool) |
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.
@rafaqz (and as fyi @nickrobinson251 ), no idea why, but including the Dates.Date
and Dates.DateTime
precompiles in Parsers.jl is causing JuliaData/CSV.jl#981, i.e. precompiling CSV.jl on windows compleletly fails. Going to revert those two type precompiles for now and we can figure out what to do afterwards.
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.
can you only disable them for Windows?
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.
Its pretty weird that that happens. We could also rewrite a few of these methods to reduce compilation overheads, which may fix the problem.
It would be great to have this back again, it was a big quality of life improvement |
We have to figure out why it causes segfaults on windows with Julia 1.7 first during precompilation. |
We could just rewrite those methods to reduce compilation time and see if that fixes it. |
This reduces ttfx of CSV.jl by 12 seconds in these benchmarks:
Current timing with main branches of Parsers.jl and CSV.jl:
With this PR and main of CSV.jl:
With the PR at CSV.jl: