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

MemoryBuffer <: AbstractVector #14

Open
Tokazama opened this issue Feb 7, 2022 · 11 comments
Open

MemoryBuffer <: AbstractVector #14

Tokazama opened this issue Feb 7, 2022 · 11 comments

Comments

@Tokazama
Copy link
Member

Tokazama commented Feb 7, 2022

Should we have MemoryBuffer be a vector or maybe act vector like, with a basic indexing interface?

@chriselrod
Copy link
Member

@brenhinkeller might've wanted something this, too.

@Tokazama
Copy link
Member Author

Tokazama commented Feb 7, 2022

It probably makes more sense for it to be array like so that we don't have to worry about complications due to indexing with other collections. But basic iteration and getindex(::MemoryBuffer, i::Int) seems nice to have.

@brenhinkeller
Copy link
Contributor

Seems like it could be handy!

@chriselrod
Copy link
Member

I do fear that adding indexing methods would encourage/steer people towards less flexible and less convenient APIs for whatever they're using it for.

Anyone using this should write their structs and method implementations around Ptr, with MemoryBuffer existing merely for the purpose of providing a Ptr.
If MemoryBuffer passes through a (non-inlined) function boundary, it will be heap allocated.
If someone wishes to avoid this, they must take the approach of GC.@preserve-ing it, and carrying the pointer with them.

This will be easy to do if they make their code based on Ptr, and involve minimal code duplication on their end.
This will not be easy to do if they make their code based on indexing into some AbstractVector, and require them add both Ptr and AbstractVector-wrapping implementations.

I think we should aim to encourage the former.
Maybe we can think of things that'd make that more ergonomic/natural?

@brenhinkeller
Copy link
Contributor

brenhinkeller commented Feb 8, 2022

If MemoryBuffer passes through a (non-inlined) function boundary, it will be heap allocated.

Oh, so this may explain one of the cases where I thought I had a StaticCompiler error that went away when I added an extra @inline

@brenhinkeller
Copy link
Contributor

brenhinkeller commented Feb 8, 2022

Back on topic, there is one case that comes to mind where it would be very useful to have getindex if nothing else, which is that for StaticTools stuff it looks it would be very useful to have a minimal stack-allocated type that can act as a vector of pointers. I.e., no need to actually subtype AbstractVector or anything, but maybe getindex(..., i::Int) and setindex!(..., x, i::Int)

@brenhinkeller
Copy link
Contributor

brenhinkeller commented Feb 8, 2022

Here's one case

function strtod(s::Ptr{UInt8}, p::Ptr{Ptr{UInt8}})
    Base.llvmcall(("""
    ; External declaration of the `strtod` function
    declare double @strtod(i8*, i8**)

    ; Function Attrs: noinline nounwind optnone ssp uwtable
    define dso_local double @main(i8* %str, i8** %ptr) #0 {
      %d = call double (i8*, i8**) @strtod (i8* %str, i8** %ptr)
      ret double %d
    }

    attributes #0 = { noinline nounwind optnone ssp uwtable }
    """, "main"), Float64, Tuple{Ptr{UInt8}, Ptr{Ptr{UInt8}}}, s, p)
end
@inline function strtod(s::AbstractMallocdMemory)
    b = MemoryBuffer{1,Ptr{UInt8}}(undef)
    d = GC.@preserve b strtod(pointer(s), pointer(b))
    return d, b
end
@inline function strtod(s)
    b = MemoryBuffer{1,Ptr{UInt8}}(undef)
    d = GC.@preserve s b strtod(pointer(s), pointer(b))
    return d, b
end

so in this example a relatively minor QoL point about being able to get p back with b[1] vs load(pointer(b)), but could become more of a QoL point in the probably-inevitable case of having more than one pointer to buffer at once

@chriselrod
Copy link
Member

Hmm, that makes sense I guess.

@brenhinkeller
Copy link
Contributor

brenhinkeller commented Feb 8, 2022

I suppose more documentation of "You don't want MemoryBuffer directly if you want an array, use (...) instead" could help avoid the possible side-effects; I could probably PR some if y'all like

@chriselrod
Copy link
Member

PRs welcome.

@brenhinkeller
Copy link
Contributor

I'll see what I can do after I catch up on some non-coding work

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

No branches or pull requests

3 participants