-
Notifications
You must be signed in to change notification settings - Fork 183
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 an offline precompile workload #1195
Conversation
dba39cc
to
86fb137
Compare
With this PR, on 1.11 (because SnoopCompile is broken on julia master)
|
Really nice, how fast this got fixed! Removing TTFX for every julia session for simple .get request is really a valuable thing in my opinion. Going to test the solution now! |
Okay, we are actually faster than CurlHTTP.jl! Also it fixed post request times! This is insane! I don't know if precompilation of all julia packages took longer, but this is lifechanging, finally get/post requests are fast in julia! |
@quinnj perhaps you're best positioned to review this? |
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.
Yeah, I think this looks pretty great.
Great. What about the I think they're robust to being called twice in a row.. i.e. |
Things work well after 1 day of usage. I guess compat is also good. |
@quinnj I don't have merge rights here |
Calling |
The time to load increase makes the total latency bigger than it was before, or? Regarding inits, those should already run, no? |
Time to load is the same. I showed time to precompile. |
Aha, the |
It is interesting that this works so well given that the test case takes the https path while precompilation is for http. |
julia> using HTTP
julia> @time HTTP.get("http://google.com");
1.416276 seconds (1.73 M allocations: 83.683 MiB, 2.23% gc time, 66.31% compilation time) julia> using HTTP
julia> @time HTTP.get("https://google.com");
2.386149 seconds (3.09 M allocations: 153.946 MiB, 2.69% gc time, 95.02% compilation time) |
Note also that HTTP compiles quite a lot depending on the input arguments, but this PR is still very benefitial for these cases too:
|
I know you're saying that its probably not necessary, but if I bake those in
Without baking the kwarg ones in
Worth it? Also, I assume it's not trivial to cover https in this offline workload? |
I don't think it is worth it, I just picked some keywords at random, and there are lots of them.
You can probably do something like Line 511 in b7f5aa8
|
Great, that works. Now
|
d03303d
to
b3e27a7
Compare
New nightly failures are some unexpected allocations
|
Merge? |
src/HTTP.jl
Outdated
@@ -631,4 +631,9 @@ function Base.parse(::Type{T}, str::AbstractString)::T where T <: Message | |||
return m | |||
end | |||
|
|||
# only run if precompiling pkgimages | |||
if VERSION >= v"1.9.0-0" && Base.JLOptions().use_pkgimages != 0 && ccall(:jl_generating_output, Cint, ()) == 1 |
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.
Yeesh; it'd be nice if PrecompileTools provided an API for all these conditions (isprecompiling
?)
# random port in the dynamic/private range (49152–65535) which are are | ||
# least likely to be used by well-known services | ||
_port = 57813 | ||
|
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.
# random port in the dynamic/private range (49152–65535) which are are | |
# least likely to be used by well-known services | |
_port = 57813 |
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.
This broke things because _port
is an arg given to serve!
, but surprisingly not 1.10, which I don't understand...
btw, I had tried do listenany
without a port arg and got a method error.
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've reverted the change. And I think the reason 1.10 didn't fail is that I think on 1.10 if coverage is requested pkgimages aren't used.
So I just made the precompile workload run whether or not pkgimages are requested, so that coverage isn't weird in tests.
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 for doing this @IanButterworth!
This reverts commit cb9b1f2.
Late comment:
Typically, the answer to this is to add |
Thanks. I took a look into adding that but it's not clear where to, and my attempts were in vain. There's a lot of functions returning functions, so it's not particularly clear where to put the annotation. @quinnj says he is close to the 2.0 rewrite being ready, so it might not be worth it, unless it jumps out to someone. |
Gotcha. I assumed it was compiling many variants of The use-case for This should probably be used more heavily in packages than it is. |
I suspect that this PR broke precompilation of HTTP on Julia 1.9. See for example: https://github.com/JuliaRegistries/General/actions/runs/12001290587/job/33451587371 Can we skip the precompile workload on Julia 1.9? |
Just a trailing note.. I was underestimating the improvement here because it turns out google.com is quite slow to resolve for me...
whereas before this PR
|
Love to see! Insane improvements guys! |
didn't see anyone showing first server response time changes #### 1.10.10
server = HTTP.serve!("127.0.0.1", 8088; ) do req
HTTP.Response(200)
end
julia> using HTTP
julia> server = HTTP.serve!("127.0.0.1", 8088; ) do req
HTTP.Response(200)
end
[ Info: Listening on: 127.0.0.1:8088, thread id: 1
shell> time curl localhost:8088
curl 'localhost:8088' 0.00s user 0.01s system 0% cpu 1.325 total
shell> time curl localhost:8088
curl 'localhost:8088' 0.01s user 0.00s system 91% cpu 0.006 total
#### 1.10.12
julia> using HTTP
julia> server = HTTP.serve!("127.0.0.1", 8088; ) do req
HTTP.Response(200)
end
[ Info: Listening on: 127.0.0.1:8088, thread id: 1
shell> time curl localhost:8088
curl 'localhost:8088' 0.00s user 0.00s system 0% cpu 0.479 total
shell> time curl localhost:8088
curl 'localhost:8088' 0.00s user 0.01s system 84% cpu 0.007 total |
Fixes #1194
Or at least makes decent progress.
Before
This PR
All the big precompiles are baked in and just one big recompile left.
Though there is an issue open about recompiles being incorrectly marked as such JuliaLang/julia#56155
Importantly, are these
__init__()
's safe to call during precompilation?