Fix IndexLens for mutable arrays #40
Conversation
Codecov Report
@@ Coverage Diff @@
## nomut #40 +/- ##
=========================================
+ Coverage 97.5% 97.61% +0.11%
=========================================
Files 4 4
Lines 160 168 +8
=========================================
+ Hits 156 164 +8
Misses 4 4
Continue to review full report at Codecov.
|
| export set, get, modify | ||
|
|
||
| import Base: get | ||
| using Base: setindex, getproperty |
There was a problem hiding this comment.
I removed setindex since we should be using our internal function _setindex everywhere and avoid future bugs due to typo.
| set(l::IndexLens, obj, val) = _setindex(obj, val, l.indices...) | ||
|
|
||
| @generated function _setindex(obj, val, indices...) | ||
| if hasmethod(Base.setindex, Tuple{obj, val, indices...}) |
There was a problem hiding this comment.
I know hasmethod is not pure and therefore is not legit inside @generated in principle. But I've been seeing code deviating from the principle and somehow magically working. My guess was that this implementation is OK as long as _setindex is called after Base.setindex(obj, ...) is defined. Since our StaticArrays tests are passing, this implementation is fine?
There was a problem hiding this comment.
Can you show a place, where people use code like this? Generally such things can cause bugs depending on import order of packages. But I don't understand enough to judge how safe this case is. I would guess that you could break this by
using Setfield
using StaticArrays
# define your own StaticArray with custom setindexOTOH if you don't make this a generated function, with the new compiler it might still perform very well. Have you tested?
There was a problem hiding this comment.
No I don't know the working code base using exactly this. I should test this and also do benchmark for the dynamic version.
BTW, what is the new compiler optimization you are talking about? The Union-splitting https://julialang.org/blog/2018/08/union-splitting sounds very close but the branching information still has to sit at the type level, right? Also, this blog post is mentioning that method lookup is still slow.
But even if @generated approach does not work, we can always have a dynamic version as a fallback and then manually implement fast non-branching version for popular types (like Array, Tuple and SArray). Probably it is not hard to get > 90% coverage by implementing it for a few types.
There was a problem hiding this comment.
I had better constant propagation in mind. And I thought hasmethod might compile down to true or false, but it does not. It does dynamically look if there is a method.
|
So my benchmark shows the static version is ~20 times faster. @generated function setindex_dynamic(obj, val, indices...)
quote
if hasmethod(Base.setindex, $(Tuple{obj, val, indices...}))
setter = Base.setindex
else
setter = setindex_on_copy
end
setter(obj, val, indices...)
end
end
@generated function setindex_static(obj, val, indices...)
if hasmethod(Base.setindex, Tuple{obj, val, indices...})
setter = Base.setindex
else
setter = setindex_on_copy
end
quote
$setter(obj, val, indices...)
end
end
function setindex_on_copy(obj, val, indices...)
clone = similar(obj, promote_type(eltype(obj), typeof(val)))
copyto!(clone, obj)
setindex!(clone, val, indices...)
return clone
end
using BenchmarkTools
a = [1, 2, 3]
println("\n", "*** setindex_dynamic")
display(@benchmark setindex_dynamic($a, 1, 1))
println("\n", "*** setindex_static")
display(@benchmark setindex_static($a, 1, 1))
using InteractiveUtils
versioninfo()Output: |
|
I tried to invoke the bug in the module TestHasMethodHack
using Test
function setindex_on_copy(obj, val, indices...)
clone = similar(obj, promote_type(eltype(obj), typeof(val)))
copyto!(clone, obj)
setindex!(clone, val, indices...)
return clone
end
struct A
x
end
Base.setindex(::A, x, ::Integer) = A(x)
@generated function setindex_static(obj, val, indices...)
if hasmethod(Base.setindex, Tuple{obj, val, indices...})
setter = Base.setindex
else
setter = setindex_on_copy
end
quote
$setter(obj, val, indices...)
end
end
@assert setindex_static([0], 1, 1) == [1] # Having it here (or commenting it out) does not change anything.
struct B
x
end
try
setindex_static(B(1), 2, 1)
catch err
@info "Ignoring" exception=err
end
Base.setindex(::B, x, ::Integer) = B(x)
struct C
x
end
Base.setindex(::C, x, ::Integer) = C(x)
@testset begin
@test setindex_static(A(0), 1, 1) == A(1)
@test_broken setindex_static(B(0), 1, 1) == B(1)
@test setindex_static(C(0), 1, 1) == C(1)
end
end # moduleOutput: |
|
Thanks for the example, I agree it is somewhat exotic. But this kind of thing comes up in REPL sessions. Is |
Nope :) It is just me trying to make test_quicktypes.jl pass.
Totally agree. Let's pull #39 then! |
|
I asked the question about |
3 participants
This is alternative to #39. If we want to handle mutable arrays (such as
Array) by always copying them, this PR provides an implementation for it. This is consistent with hownomutbranch handlesmutable struct.