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

add basic precompilation #337

Merged
merged 2 commits into from
Feb 8, 2022
Merged

add basic precompilation #337

merged 2 commits into from
Feb 8, 2022

Conversation

rafaqz
Copy link
Contributor

@rafaqz rafaqz commented Feb 4, 2022

This tiny PR Improves the ttfx time of Blink.jl by 5 seconds. Probably also helps other packages that use JSON.jl

Current master here and with Blink.jl:

julia> @time using Blink
  5.583824 seconds (17.56 M allocations: 740.126 MiB, 6.44% gc time, 88.95% compilation time)

julia> @time Window()
 12.164188 seconds (28.92 M allocations: 1.293 GiB, 3.17% gc time, 98.19% compilation time)
Window(1, Electron(Process(`/home/raf/.julia/dev/Blink/deps/atom/electron /home/raf/.julia/dev/Blink/src/AtomShell/main.js port 3466`, ProcessRunning), Sockets.TCPSocket(RawFD(22) active, 0 bytes waiting), Dict{String, Any}("callback" => Blink.var"#1#2"())), Page(1, WebSocket(server, CONNECTED), Dict{String, Any}("webio" => Blink.AtomShell.var"#22#23"{Blink.AtomShell.WebIOBlinkComm}(Blink.AtomShell.WebIOBlinkComm(Window(#= circular reference @-5 =#))), "callback" => Blink.var"#1#2"()), Distributed.Future(1, 1, 1, Some(true))), Task (done) @0x00007fdcd5809cd0)

This PR:

julia> @time using Blink
  5.477089 seconds (17.55 M allocations: 739.919 MiB, 6.47% gc time, 88.74% compilation time)

julia> @time Window()
  6.973419 seconds (14.42 M allocations: 779.433 MiB, 3.20% gc time, 97.07% compilation time)
Window(1, Electron(Process(`/home/raf/.julia/dev/Blink/deps/atom/electron /home/raf/.julia/dev/Blink/src/AtomShell/main.js port 4505`, ProcessRunning), Sockets.TCPSocket(RawFD(22) active, 0 bytes waiting), Dict{String, Any}("callback" => Blink.var"#1#2"())), Page(1, WebSocket(server, CONNECTED), Dict{String, Any}("webio" => Blink.AtomShell.var"#22#23"{Blink.AtomShell.WebIOBlinkComm}(Blink.AtomShell.WebIOBlinkComm(Window(#= circular reference @-5 =#))), "callback" => Blink.var"#1#2"()), Distributed.Future(1, 1, 1, Some(true))), Task (done) @0x00007f462344ab30)

@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #337 (40a2d28) into master (f93688a) will decrease coverage by 0.87%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   99.51%   98.63%   -0.88%     
==========================================
  Files           5        5              
  Lines         410      441      +31     
==========================================
+ Hits          408      435      +27     
- Misses          2        6       +4     
Impacted Files Coverage Δ
src/JSON.jl 20.00% <0.00%> (-80.00%) ⬇️
src/Parser.jl 100.00% <0.00%> (ø)
src/specialized.jl 100.00% <0.00%> (ø)
src/Writer.jl 98.26% <0.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f93688a...40a2d28. Read the comment docs.

@KristofferC
Copy link
Member

KristofferC commented Feb 4, 2022

That's crazy. What is it that takes so much time to compile? Must be Parsers.jl?

Edit: Before #285 _precompile_() took 0.25 seconds, now it takes 4 seconds. 😬

@rafaqz
Copy link
Contributor Author

rafaqz commented Feb 4, 2022

I have another PR over at Parser.jl that has a similar crazy effect on CSV.jl JuliaData/Parsers.jl#108

Something going on...

@rafaqz
Copy link
Contributor Author

rafaqz commented Feb 5, 2022

The combined effect of both of these PRs on Blink.jl

julia> @time using Blink
  2.005740 seconds (3.79 M allocations: 255.856 MiB, 7.74% gc time, 68.09% compilation time)

julia> @time Window()
  6.817974 seconds (14.42 M allocations: 779.489 MiB, 3.63% gc time, 97.04% compilation time)
Window(1, Electron(Process(`/home/raf/.julia/dev/Blink/deps/atom/electron /home/raf/.julia/dev/Blink/src/AtomShell/main.js port 5005`, ProcessRunning), Sockets.TCPSocket(RawFD(22) active, 0 bytes waiting), Dict{String, Any}("callback" => Blink.var"#1#2"())), Page(1, WebSocket(server, CONNECTED), Dict{String, Any}("webio" => Blink.AtomShell.var"#22#23"{Blink.AtomShell.WebIOBlinkComm}(Blink.AtomShell.WebIOBlinkComm(Window(#= circular reference @-5 =#))), "callback" => Blink.var"#1#2"()), Distributed.Future(1, 1, 1, Some(true))), Task (done) @0x00007f1e570f9cd0)

So precompilation here is still catching something not covered in Parsers.jl precompilation.

@rafaqz
Copy link
Contributor Author

rafaqz commented Feb 8, 2022

Can we get this merged?

@KristofferC
Copy link
Member

Tests fail due to the _precompile_() in the test. Not sure why it is there at all.

@rafaqz
Copy link
Contributor Author

rafaqz commented Feb 8, 2022

Oh right I pushed that after asking to merge 😅

Was just looking to get rid of missing code coverage lines. What do you think?

@KristofferC
Copy link
Member

It runs during precompilation so it is tested. Imo just remove it.

@KristofferC KristofferC merged commit b5ca9aa into JuliaIO:master Feb 8, 2022
@rafaqz rafaqz deleted the precompile branch February 8, 2022 14:17
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.

3 participants