use JSON3 to fix type instability and improve performance #15
Conversation
|
🎉 Using tuples in JSON is really unidiomatic but that's a bigger change that might happen at some point in the future (along with putting the file in some other place than your home directory). |
|
Well, it already used tuples but JSON.jl returned |
twavv
left a comment
twavv
left a comment
There was a problem hiding this comment.
LGTM, can you address the couple of review comments? Happy to merge/release after.
src/AssetRegistry.jl
Outdated
| # get existing information | ||
| prev_registry = filesize(io) > 0 ? JSON.parse(io) : Dict{String,Tuple{String, Int}}() | ||
| DT = Dict{String,Tuple{String,Int}} | ||
| prev_registry = filesize(io) > 0 ? JSON3.read(io, DT) : DT() |
There was a problem hiding this comment.
Can we not repeat this logic? (e.g., extract this into its own function to read the registry from disk and return it as a dict)
There was a problem hiding this comment.
I refactored the code shared between register and deregister into update_registry_file
Co-authored-by: Travis DePrato <773453+travigd@users.noreply.github.com>
|
Can we get this merged? |
|
Actually this is better done with a precompile statement in JSON.jl, so closing. |
|
No, turns out this PR is better and I cant beat the above reduction with precompilation in JSON |
2 participants
The compile time of Interact.jl (and probably other packages that use AssetRegistry) is hugely affected by JSON serialisation of the asset registry. Currently its not type stable, because JSON does not detect tuples, but mixed type vectors (the areas with the label are AssetRregistry.jl):
Using JSON 3 and specifying the type lets us use tuple:
These are the flame graphs for
@profile Interact.slider(1:10). The time goes from over 15 seconds on my laptop on master, to 5-7 seconds with this PR.