Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow mutation of non-isbits eltypes #1150

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MasonProtter
Copy link

@MasonProtter MasonProtter commented Apr 3, 2023

For large MArrays, this'll be slower than if we had a proper MutableTuple, but it does work and should be pretty quick for reasonable sizes. It shouldn't screw up the garbage collector or do anything unsafe like the previous version did.

@MasonProtter
Copy link
Author

That's so bizarre, this test works totally fine for me locally, and the operations it's doing should be legal.

serenity4 added a commit to serenity4/StaticArrays.jl that referenced this pull request Nov 18, 2023
serenity4 added a commit to serenity4/StaticArrays.jl that referenced this pull request Nov 18, 2023
@serenity4
Copy link

serenity4 commented Nov 18, 2023

This works for me too, perhaps we can give it another try? What were the issues? (logs are no longer available)

I made an attempt as well, with some special-casing for certain mutable types (for which I believe the concerns of #27 do not apply): master...serenity4:setindex-nonisbits. That seems to greatly improve performance for large arrays:

julia> mutable struct Mut; x::Float64; end

julia> @btime setindex!(v, $("ho"), 2) setup = v = @MArray ["ha" for i in 1:100];
  408.465 ns (3 allocations: 1.62 KiB)

julia> @btime setindex!(v, $(Mut(3.0)), 2) setup = v = @MArray [Mut(rand()) for i in 1:100];
  7.032 ns (0 allocations: 0 bytes)

Though this optimization could be implemented later and if we are really 100% sure it won't cause any issues. I'd just try to go forward with the current PR first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants