-
Notifications
You must be signed in to change notification settings - Fork 9
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
move bounds checks to constructor #32
base: main
Are you sure you want to change the base?
Changes from 9 commits
e11a7f9
0dbcae5
0aff1f0
35b22f6
0627afa
7bf6830
effbc95
d70026c
f2e114c
03ec596
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,58 @@ | ||
struct InvalidBlobError <: Exception | ||
type::Type | ||
base::Ptr{Nothing} | ||
offset::Int64 | ||
limit::Int64 | ||
length::Int64 | ||
end | ||
|
||
function Base.showerror(io::IO, e::InvalidBlobError) | ||
print(io, "InvalidBlobError: $(e.type) needs $(e.length) * $(self_size(e.type)) bytes. \ | ||
Got length($(e.offset):$(e.limit)) == $(e.limit - e.offset) bytes") | ||
end | ||
|
||
""" | ||
A pointer to a `T` stored inside a Blob. | ||
Blob{T} | ||
|
||
A pointer to a memory array that stores a `T`. | ||
|
||
The fields are stored compact in memory without alignment, i.e each basic field f takes up | ||
`sizeof(fieldtype(T, :f))` bytes. Blobs inside `T` take just the offset, i.e. 8 bytes. | ||
This is different from Julia memory layout. | ||
|
||
You can just store struct of only primitive types or structs of primitive out of the box, | ||
for example: | ||
|
||
```julia | ||
struct Foo | ||
x::Int64 | ||
y::Float64 | ||
end | ||
|
||
blob = Blobs.malloc(Foo) | ||
blob[] = Foo(42, 3.14) | ||
|
||
In order to store variable size data structures (`BlobVector`, `BlobBitVector`, | ||
`BlobString` or your own implementation) or a `Blob``, you need to implement `child_size` | ||
and `init` for your type. | ||
|
||
Example: | ||
```julia | ||
struct FooString | ||
s::BlobString | ||
i::Int64 | ||
end | ||
|
||
function Blobs.child_size(::FooString, string_length::Int64) | ||
return child_size(BlobString, string_length) | ||
end | ||
|
||
function Blobs.init(blob::Blob{FooString}, free::Blob{Nothing}, string_length::Int64) | ||
free = Blobs.init(blob.s, free, string_length) | ||
blob.i[] = 0 | ||
return free | ||
end | ||
``` | ||
""" | ||
struct Blob{T} | ||
base::Ptr{Nothing} | ||
|
@@ -8,54 +61,99 @@ struct Blob{T} | |
|
||
function Blob{T}(base::Ptr{Nothing}, offset::Int64, limit::Int64) where {T} | ||
@assert isbitstype(T) | ||
new(base, offset, limit) | ||
@boundscheck _bounds_check(base, offset, limit, self_size(T), T) | ||
new{T}(base, offset, limit) | ||
end | ||
end | ||
|
||
@noinline function _bounds_check( | ||
base::Ptr{Nothing}, | ||
offset::Int64, | ||
limit::Int64, | ||
self_size_T::Int64, | ||
@nospecialize(T::DataType)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this Julia doesn't specialize arguments that look like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, good catch. I added it because I was surprised that there is still more code generated by the constructor (I believe). But it didn't change anything for the reason that you point out. |
||
if offset < 0 || offset + self_size_T > limit | ||
throw(InvalidBlobError(Blob{T}, base, offset, limit, 1)) | ||
end | ||
if limit > 0 && base == Ptr{Nothing}(0) | ||
throw(AssertionError("Null pointer reference Blob{$(T)}")) | ||
end | ||
end | ||
|
||
""" | ||
Blob{T}(ref::Base.RefValue{T}) where T | ||
|
||
Create a `Blob{T}` from an Julia allocated object. | ||
**Danger**: This only works if memory layout of Julia struct is the same as of the Blob. | ||
""" | ||
function Blob(ref::Base.RefValue{T}) where T | ||
Blob{T}(pointer_from_objref(ref), 0, sizeof(T)) | ||
@assert self_size(T) == sizeof(T) "$(T) cannot of aligned fields or Blobs" | ||
@inbounds Blob{T}(pointer_from_objref(ref), 0, self_size(T)) | ||
end | ||
|
||
""" | ||
Blob{T}(base::Ptr{T}, offset::Int64 = 0, limit::Int64 = sizeof(T)) where T | ||
|
||
Create a `Blob{T}` from a pointer. | ||
""" | ||
Base.@propagate_inbounds \ | ||
function Blob(base::Ptr{T}, offset::Int64 = 0, limit::Int64 = sizeof(T)) where {T} | ||
Blob{T}(Ptr{Nothing}(base), offset, limit) | ||
end | ||
|
||
function Blob{T}(blob::Blob) where T | ||
""" | ||
Blob{T}(blob::Blob) | ||
|
||
Make a copy and potentially change the type of a `Blob`. | ||
""" | ||
Base.@propagate_inbounds function Blob{T}(blob::Blob) where T | ||
Blob{T}(getfield(blob, :base), getfield(blob, :offset), getfield(blob, :limit)) | ||
robertbuessow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
""" | ||
available_size(blob::Blob{T}) where T | ||
|
||
The size of memory this `Blob` and it's children own. `blob.limit - blob.offset`. | ||
""" | ||
available_size(blob::Blob{T}) where T = getfield(blob, :limit) - getfield(blob, :offset) | ||
|
||
function assert_same_allocation(blob1::Blob, blob2::Blob) | ||
@assert getfield(blob1, :base) == getfield(blob2, :base) "These blobs do not share the same allocation: $blob1 - $blob2" | ||
end | ||
|
||
"""" | ||
pointer(blob::Blob{T}) where T | ||
|
||
Get a pointer to the data in the `blob`. Note that you cannot `unsafe_load` | ||
from this pointer, since the data is not aligned. | ||
""" | ||
function Base.pointer(blob::Blob{T}) where T | ||
convert(Ptr{T}, getfield(blob, :base) + getfield(blob, :offset)) | ||
end | ||
|
||
function Base.:+(blob::Blob{T}, offset::Integer) where T | ||
"""" | ||
Base:+(::Blob, ::Integer) | ||
|
||
Increase the offset of a `Blob` by `offset`. | ||
""" | ||
Base.@propagate_inbounds function Base.:+(blob::Blob{T}, offset::Integer) where T | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's traditional in the C world to allow people to construct a pointer that goes just off the end of an array. (But not to access that pointer) This code prohibits the construction of the pointer. Is that where we want to go? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can created a |
||
Blob{T}(getfield(blob, :base), getfield(blob, :offset) + offset, getfield(blob, :limit)) | ||
end | ||
|
||
""" | ||
Base:-(::Blob, ::Blob) | ||
|
||
Get the offset difference of two blobs in the same allocation. | ||
""" | ||
function Base.:-(blob1::Blob, blob2::Blob) | ||
assert_same_allocation(blob1, blob2) | ||
getfield(blob1, :offset) - getfield(blob2, :offset) | ||
end | ||
|
||
@inline function boundscheck(blob::Blob{T}) where T | ||
@boundscheck begin | ||
if (getfield(blob, :offset) < 0) || (getfield(blob, :offset) + self_size(T) > getfield(blob, :limit)) | ||
throw(BoundsError(blob)) | ||
end | ||
@assert (getfield(blob, :base) != Ptr{Nothing}(0)) "Null pointer dereference in $(typeof(blob))" | ||
end | ||
end | ||
|
||
Base.@propagate_inbounds function Base.getindex(blob::Blob{T}) where T | ||
boundscheck(blob) | ||
function Base.getindex(blob::Blob{T}) where T | ||
unsafe_load(blob) | ||
end | ||
|
||
# TODO(jamii) do we need to align data? | ||
""" | ||
self_size(::Type{T}, args...) where {T} | ||
|
||
|
@@ -88,22 +186,21 @@ end | |
|
||
@generated function Base.getindex(blob::Blob{T}, ::Type{Val{field}}) where {T, field} | ||
i = findfirst(isequal(field), fieldnames(T)) | ||
@assert i != nothing "$T has no field $field" | ||
@assert i !== nothing "$T has no field $field" | ||
quote | ||
$(Expr(:meta, :inline)) | ||
Blob{$(fieldtype(T, i))}(blob + $(blob_offset(T, i))) | ||
@inbounds Blob{$(fieldtype(T, i))}(blob) + $(blob_offset(T, i)) | ||
end | ||
end | ||
|
||
@inline function Base.getindex(blob::Blob{T}, i::Int) where {T} | ||
@boundscheck if i < 1 || i > fieldcount(T) | ||
throw(BoundsError(blob, i)) | ||
end | ||
return Blob{fieldtype(T, i)}(blob + Blobs.blob_offset(T, i)) | ||
return @inbounds Blob{fieldtype(T, i)}(blob) + Blobs.blob_offset(T, i) | ||
end | ||
|
||
Base.@propagate_inbounds function Base.setindex!(blob::Blob{T}, value::T) where T | ||
boundscheck(blob) | ||
unsafe_store!(blob, value) | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for making this
@noinline
? Julia doesn't do it forcheckbounds(::AbstractArray, I...)
, for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the large amount of generate llvm code was the bounds check (building the strings etc). Therefore, I want to avoid that this is getting inlined into the specialized function.