jw3126 / Setfield.jl

Update deeply nested immutable structs.
Other
167 stars 17 forks source link

Add ConstIndexLens for type-stable indexing of tuples #60

Closed tkf closed 5 years ago

tkf commented 5 years ago

This PR adds a new lens ConstIndexLens constructible via @lens _[$1] etc.

Not sure if it should go directly to public API. Is it better to put it in experimental? Although the sugar @lens _[$1] is kind of nice to have (but then it's not possible to hide it in experimental module).

fixes #59

jw3126 commented 5 years ago

Thanks for looking into this! I am not against this, but I want to ponder the options a bit.

tkf commented 5 years ago

Is this type instability a problem you encountered in real code? Can you give such an example?

Sure. I think it happens generally when you store IndexLens somewhere (e.g., inside some object) and use it somewhere else (which is far away enough so that the constant can't propagate):

using Setfield

sweeper = (
    model = (1, 2.0, 3im),
    axis = (@lens _[2]),
)

function f(s)
    a = sum(set(s.model, s.axis, 0))
    for i in 1:10
        a += sum(set(s.model, s.axis, i))
    end
    return a
end

then

julia> @code_warntype f(sweeper)
Body::Any
...

Strangely, in this case putting the construction of the lens inside a function does not help:

julia> function g(x)
           sweeper = (
               model = (1, x, 3im),
               axis = (@lens _[2]),
           )
           return f(sweeper)
       end
g (generic function with 1 method)

julia> @code_warntype g(2.0)
Body::Any
...

But I realized that an alternative solution to ConstIndexLens would be to store the standard lens instance in a type parameter like Val does. This probably is enough for now. However, if we support obj[end] via closure-based approach and if the closure is happened to include non-isbit type (I guess?), I can't put it in the type parameter. But maybe this case is rare enough.

One thing that might help the compiler would be to use an approach as sketched here for IndexLens.

Isn't closure implemented as a callable struct? (see getfield for n below):

julia> h = let n = 1
           x -> x^n
       end
#13 (generic function with 1 method)

julia> @code_warntype h(2)
Body::Int64
1 ─ %1 = (Core.getfield)(#self#, :n)::Int64
│   %2 = invoke Base.power_by_squaring(_2::Int64, %1::Int64)::Int64
└──      return %2

So my guess is that it would be only as efficient as current IndexLens. But OOPLens itself is worth implementing it for its own sake. Maybe compiler would start storing isbit types as constants in closure someday... Then indeed we would get this feature "for free". I wonder how hard it is to put it in core Julia (i.e., convincing core devs + implement a PR).

tkf commented 5 years ago

As for the "real world" example, I needed this in flat branch of Transducers.jl to gain some type-stability: https://github.com/tkf/Transducers.jl/commit/5ae826a8a9acce1e0a0feb497a6f49da1a252231#diff-61efac4de4204dbab82023b75c3143b5 but I then ditched that branch as I couldn't make it fast.

jw3126 commented 5 years ago

This is probably more or less your Val idea:

using Setfield

struct StaticLens{l} <: Lens end

static(l::Lens) = StaticLens{l}()

Setfield.set(o, ::StaticLens{l}, val) where {l} = set(o, l, val)

sweeper = (
    model = (1, 2.0, 3im),
    axis = static(@lens _[2]),
)

function f(s)
    a = sum(set(s.model, s.axis, 0))
    for i in 1:10
        a += sum(set(s.model, s.axis, i))
    end
    return a
end

@code_warntype f(sweeper)

It gives

Body::Union{Complex{Int64}, Int64}

This is much better then the above Any and the compiler also gets rid of Float64. But still it is not fully optimized. Does this PR optimize the sweeper example correctly? Might be worth adding to the tests.

jw3126 commented 5 years ago

Just checked, the sweeper example is correctly optimized with this PR. Still a good test case.

codecov-io commented 5 years ago

Codecov Report

Merging #60 into master will increase coverage by 4.66%. The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   87.91%   92.57%   +4.66%     
==========================================
  Files           4        5       +1     
  Lines         182      202      +20     
==========================================
+ Hits          160      187      +27     
+ Misses         22       15       -7
Impacted Files Coverage Δ
src/sugar.jl 90.27% <80%> (+13.35%) :arrow_up:
src/lens.jl 96.07% <83.33%> (-3.93%) :arrow_down:
src/settable.jl 91.66% <0%> (-1.67%) :arrow_down:
src/Setfield.jl 100% <0%> (ø)
src/experimental.jl 94.44% <0%> (+11.11%) :arrow_up:

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 61774fe...2fecafe. Read the comment docs.

tkf commented 5 years ago

Cool. Thanks for the review. I added tests with @set bd59770cd33fe6df99d02f13881ecbc349b20b2c and sweeper example 2fecafe2ae43ef87804e726bf49bf001c1e4a162.

jw3126 commented 5 years ago

Thanks for this PR! High quality as always. Do you want me to tag a new release?

tkf commented 5 years ago

Hooray! :rocket: https://github.com/JuliaLang/METADATA.jl/pull/21035