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

the GenericMemory type parameters #54312

Open
nsajko opened this issue Apr 30, 2024 · 1 comment
Open

the GenericMemory type parameters #54312

nsajko opened this issue Apr 30, 2024 · 1 comment
Labels
arrays [a, r, r, a, y, s] breaking This change will break code design Design of APIs or of the language itself speculative Whether the change will be implemented is speculative

Comments

@nsajko
Copy link
Contributor

nsajko commented Apr 30, 2024

#53854 (comment) @oscardssmith

We know that regular Memory has the interface we want, but I think we wanted a release or two and a few examples using the addrspace and kind before we actually decide that the GenericMemory doesn't have any design defects that would require breaking changes if it was part of the interface.

The order of the parameters: could the element type be moved to the front?

GenericMemory has three type parameters, of which the element type is set in stone, as it's required by the AbstractVector supertype; the remaining two parameters seem subject to change. I think it would make sense to make the element type the first type parameter, and perhaps declare in the docs that the rest of the type parameters are experimental/subject to change. This would make it more convenient to use GenericMemory as a type parameter in user types while avoiding the non-public type parameters of GenericMemory.

For example, the FixedSizeArrays package (not registered yet), is currently backed by Memory storage, but it'd ideally support GenericMemory: JuliaArrays/FixedSizeArrays.jl#33. Currently the struct definition looks like this (ref):

struct Internal end

struct FixedSizeArray{T,N} <: DenseArray{T,N}
    mem::Memory{T}
    size::NTuple{N,Int}
    function FixedSizeArray{T,N}(::Internal, mem::Memory{T}, size::NTuple{N,Int}) where {T,N}
        new{T,N}(mem, size)
    end
end

GenericMemory support could look something like this:

struct FixedSizeArray{T,N,Mem<:GenericMemory{<:Any,T}} <: DenseArray{T,N}
    mem::Mem
    size::NTuple{N,Int}
    function FixedSizeArray{T,N,Mem}(
        ::Internal, mem::Mem, size::NTuple{N,Int},
    ) where {T,N,Mem<:GenericMemory{<:Any,T}}
        new{T,N,Mem}(mem, size)
    end
end

Moving the element type parameter of GenericMemory to the first place would enable avoiding the awkward <:Any above.

The kind parameter: could it be made a singleton type instead of a Symbol value?

Having something like this, unlike just Symbols, would be typo-proof:

module MemoryKind
  struct Atomic end
  struct NotAtomic end
end

const Memory = GenericMemory{T,MemoryKind.NotAtomic,CPU} where {T}
@nsajko nsajko added the design Design of APIs or of the language itself label Apr 30, 2024
@oscardssmith
Copy link
Member

The kind change seems reasonable to me. That was one of the later pieces added to the design , although one advantage of symbols is that the C code needs to create them so doing more complicated things is annoying.

The type parameter ordering was very deliberate. The reasoning is that most code that works with GenericMemory will be fairly generic on the eltype, but the atomic memory will usually require fairly different code because all the mutations need to supply a memory order.

@nsajko nsajko added arrays [a, r, r, a, y, s] breaking This change will break code speculative Whether the change will be implemented is speculative labels Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] breaking This change will break code design Design of APIs or of the language itself speculative Whether the change will be implemented is speculative
Projects
None yet
Development

No branches or pull requests

2 participants