RFC: setindex! for non-isbits MArrays #749
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark resultJudge resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
c42f
force-pushed
the
cjf/setindex-MArray
branch
from
c1e41aa
to
4614a00
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/StaticArrays.jl/StaticArrays.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
andyferris
reviewed
Feb 27, 2020
There was a problem hiding this comment.
Can we use @GC.preserve on both v and val and use something like the isbitstype path? We basically want to copy a pointer... so long as the garbage collection is conservative enough this should be safe, no? Or is there something about the generational GC that makes this more complex than I would guess?
(We should split out the convert to earlier so that the pointer copy part is provably synchronous)
|
For the "fail hard" option, wouldn't it be better to disallow even creating
Shouldn't people who wants to optimize the hell out of their code use (@KristofferC If the benchmark comment from the bot is too noisy, we can turn off the comment. See #741) |
Well I kind of think not? The storage Related - I'm not exactly sure why we get away with apparently the exact same thing for bits types! (I guess the inlining of "immutable" storage into the mutable parent structure has some weird semantic consequences.)
Yes! The GC is generational. Objects are split into old and new generations. In order to collect the new generation independently, it needs to specially track any old objects which point to new ones. This means you need to inform the GC whenever you set a pointer field (so it can update the internal "remset" if the object is old and the child pointed to is new). This is the purpose of the obscurely named runtime function |
You might be right about that. We currently have another bug where initializing with So perhaps this PR would have better been an issue: "What to do about MArray with non-isbits elements?"
Temporary When I say "extreme lengths" I guess it's a matter of perspective: I find imperative in-place numerical linear algebra more natural to reason about compared to the very functional style that SArray demands.
Yes I think it's a bit noisy (especially when I do stupid things like breaking the tests, as I did here 😬) I've merged that now to reduce notification volume. |
I guess this is what I meant to say by "optimize the hell out." I wanted to mention that By the way, can the |
Excellent question, I checked the data layout and I think the answer is a conclusive "no for now" because tuples-of-unions are not inlined. Indeed I think the covariance of tuples makes this impossible right now because a data storage Possibly it would help to have systematic treatment of small union selector bits along the lines of JuliaLang/julia#33120 but I guess we would still need a new builtin |
Actually very recently we have JuliaLang/julia#34126 — if that were to merge we might actually be in a position to fix this without a huge performance hit. Though we'd still need to figure out how to maintain correct GC invariants. |
Can we just insert a "value wrapper" to get an invariant tuple type? struct InvariantValue{T} # "value wrapper"
value::T
end
struct InvariantTuple{T <: Tuple{Vararg{InvariantValue}}}
values::T
end
Base.getindex(t::InvariantTuple, i::Integer) = t.values[i].value
# Some helper functions
(::Base.Colon)(x::S, ::Type{T}) where {T, S <: T} = InvariantValue{T}(x)
invariant(args::InvariantValue...) = InvariantTuple(args)
t1 = invariant(1:Union{Missing,Int}, 2.0:Union{Missing,Float64})
t2 = invariant(1:Union{Missing,Int}, missing:Union{Missing,Float64})
typeof(t1) === typeof(t2)(Alternatively, we can also create a "typed linked list." Though I guess this is much more compiler-friendly.) I guess what we need here is actually struct InvariantNTuple{N,T}
values::NTuple{N,InvariantValue{T}}
endwhich may be more compiler friendly (naively thinking, fewer type parameters sounds good)? |
Implement this for the sake of people using MArray in generic code, even though it's unlikely to be fast.
There's not a lot of point testing MVector/MMatrix separately, as they're now the same struct (since Julia 0.6 or so)
|
Can someone comment on the status of this PR? I find myself in the "people wanting to write generic code using MArray" group described above, particularly to keep open the option of using BigFloats. |
It merged, so it should be fine now? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here I've tried implementing this for the sake of people using MArray in generic code, even though it's unlikely to be fast. (Furthermore I can't think of a way to make it fast. It's like we need a weird kind of mutable pseudo-container which is guaranteed to be inlined as a part of the parent struct, such that you can't access it separately. Ironically a mutable
FieldArrayshould work like this.)Overall I'm somewhat inclined to do this but not decided. If we leave this as-is, it doesn't leave a good option for people wanting to write generic code using
MArray. It seems we can have:BigFloator suchlike on occasion.SizedArray. Good for aspiring performance hackers but kind of bad for everyone else.The generated code for this isn't great. Basically
m[i] = vturns into a big pile of(ifelse(i == 1, v, m[1]), ifelse(i == 2, v, m[2]), ...)to construct a new tuple whichm.datawill be set to.Thoughts?