Trixi.jl icon indicating copy to clipboard operation
Trixi.jl copied to clipboard

deprecate at-unpack

Open ranocha opened this issue 3 years ago • 4 comments

Since we already require Julia v1.7 where the destructuring syntax (; a, b) = stuff was introduced, I changed all uses of @unpack. This reduces the number of external functions from UnPack.jl that need to be cached, see also https://github.com/SciML/OrdinaryDiffEq.jl/pull/1893. In our case, the impact was less significant since we don't really precompile stuff, but the size of our cache file went down a bit.

Current main branch: ll ~/.julia/compiled/v1.9/Trixi gives

2416516 Mär  9 05:48 fRRHt_BQSYv.ji
25127736 Mär  9 05:48 fRRHt_BQSYv.so*

This PR yields

2413604 Mär  9 05:52 fRRHt_BQSYv.ji
24973976 Mär  9 05:52 fRRHt_BQSYv.so*

This is a reduction by ca. 150 KiB.

julia> 25127736 - 24973976
153760

julia> ans / 1024
150.15625

It looks like the automated approach

julia> function modify(filename)
           file = read(filename, String) |> strip
           file = replace(file, r"@unpack (.+) = (.+)\n" => s"(; \1) = \2\n")
           open(filename, "w") do io
               println(io, file)
           end
       end
modify (generic function with 1 method)

julia> for (root, dirs, files) in walkdir(@__DIR__)
           for file in files
               endswith(file, ".jl") || endswith(file, ".md") || continue
               filename = joinpath(root, file)
               modify(filename)
           end
       end

also introduced some whitespace changes :see_no_evil:

ranocha avatar Mar 09 '23 05:03 ranocha

Alternatively, you could replace UnPack with SimpleUnPack (that was done e.g. in OrdinaryDiffEq) and only change the Project.toml + import statements. On Julia >= 1.7, SimpleUnPack.@unpack is expanded to the (; ...) = ... syntax and on older Julia versions it expands to a block of getproperty calls (i.e., it fixes the problems with number of specializations also on older Julia versions). Similarly, SimpleUnPack.@pack! expands to a sequence of setproperty! calls.

devmotion avatar Mar 12 '23 13:03 devmotion

Alternatively, you could replace UnPack with SimpleUnPack (that was done e.g. in OrdinaryDiffEq) and only change the Project.toml + import statements. On Julia >= 1.7, SimpleUnPack.@unpack is expanded to the (; ...) = ... syntax and on older Julia versions it expands to a block of getproperty calls (i.e., it fixes the problems with number of specializations also on older Julia versions). Similarly, SimpleUnPack.@pack! expands to a sequence of setproperty! calls.

This sounds like an interesting approach as well, and probably much less hassle. Does it have the same positive impact on precompilation times?

sloede avatar Mar 12 '23 13:03 sloede

I haven't benchmarked it but that's the main idea - it does just rewrite everything using the base syntax (if available) or getproperty/setproperty! calls, so there is no difference in the generated code compared with writing out the assignments manually. Whereas UnPack.jl rewrites the assignments as calls to unpack(x, Val(key)) and pack!(x, Val(key), val) which leads to a large number of method specializations due to Val (and is also not completely equivalent to getproperty/setproperty!, e.g., unpack/pack! is implemented for dictionaries as well).

devmotion avatar Mar 12 '23 13:03 devmotion

Let's do both: I switched to SimpleUnPack.jl for now. Nevertheless, we can keep the change to Julia's standard destructuring syntax. My reasoning is that it's better to use plain base syntax when possible without too much hassle.

ranocha avatar Mar 16 '23 11:03 ranocha