Skip to content

Array resizing fixes and clean up #16893

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 29, 2016
Merged

Array resizing fixes and clean up #16893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16f682c
Array micro optimizations
yuyichao Jun 15, 2016
91488ca
A few Cstring related fix
yuyichao Jun 15, 2016
5d524ec
Implement implicit byte checker in C.
yuyichao Jun 16, 2016
cee4cb7
Implement `cconvert(Cstring)` in C
yuyichao Jun 16, 2016
d23890f
Array resizing fixes and clean up
yuyichao Jun 16, 2016
dd3ba9b
Implement faster and safer `unsafe_copy!` in C
yuyichao Jun 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 29 additions & 103 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ end
## copy ##

function unsafe_copy!{T}(dest::Ptr{T}, src::Ptr{T}, n)
# Do not use this to copy data between pointer arrays.
# It can't be made safe no matter how carefully you checked.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For someone like me not in this business, this comment is a bit frightening. Basically when I use this function, dest and src point to arrays, in the C meaning, so I interpret this comment as "never use this function", because my goal is precisely to copy data between "pointer arrays". So it would be useful to clarify your warning, and to put it in the docstring, which is more visible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, you meant arrays of pointer! still, would be worth it to move the warning in the docstring.

ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, UInt),
dest, src, n*sizeof(T))
return dest
Expand All @@ -45,9 +47,8 @@ function unsafe_copy!{T}(dest::Array{T}, doffs, src::Array{T}, soffs, n)
if isbits(T)
unsafe_copy!(pointer(dest, doffs), pointer(src, soffs), n)
else
for i=0:n-1
@inbounds arrayset(dest, src[i+soffs], i+doffs)
end
ccall(:jl_array_ptr_copy, Void, (Any, Ptr{Void}, Any, Ptr{Void}, Int),
dest, pointer(dest, doffs), src, pointer(src, soffs), n)
end
return dest
end
Expand All @@ -63,11 +64,7 @@ end

copy!{T}(dest::Array{T}, src::Array{T}) = copy!(dest, 1, src, 1, length(src))

function copy(a::Array)
b = similar(a)
ccall(:memcpy, Ptr{Void}, (Ptr{Void}, Ptr{Void}, UInt), b, a, sizeof(a))
return b
end
copy{T<:Array}(a::T) = ccall(:jl_array_copy, Ref{T}, (Any,), a)

function reinterpret{T,S}(::Type{T}, a::Array{S,1})
nel = Int(div(length(a)*sizeof(S),sizeof(T)))
Expand Down Expand Up @@ -389,66 +386,13 @@ setindex!{T, N}(A::Array{T, N}, x::Number, ::Vararg{Colon, N}) = fill!(A, x)

# efficiently grow an array

function _growat!(a::Vector, i::Integer, delta::Integer)
n = length(a)
if i < div(n,2)
_growat_beg!(a, i, delta)
else
_growat_end!(a, i, delta)
end
return a
end

function _growat_beg!(a::Vector, i::Integer, delta::Integer)
ccall(:jl_array_grow_beg, Void, (Any, UInt), a, delta)
if i > 1
ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, Csize_t),
pointer(a, 1), pointer(a, 1+delta), (i-1)*elsize(a))
end
return a
end

function _growat_end!(a::Vector, i::Integer, delta::Integer)
ccall(:jl_array_grow_end, Void, (Any, UInt), a, delta)
n = length(a)
if n >= i+delta
ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, Csize_t),
pointer(a, i+delta), pointer(a, i), (n-i-delta+1)*elsize(a))
end
return a
end
_growat!(a::Vector, i::Integer, delta::Integer) =
ccall(:jl_array_grow_at, Void, (Any, Int, UInt), a, i - 1, delta)

# efficiently delete part of an array

function _deleteat!(a::Vector, i::Integer, delta::Integer)
n = length(a)
last = i+delta-1
if i-1 < n-last
_deleteat_beg!(a, i, delta)
else
_deleteat_end!(a, i, delta)
end
return a
end

function _deleteat_beg!(a::Vector, i::Integer, delta::Integer)
if i > 1
ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, Csize_t),
pointer(a, 1+delta), pointer(a, 1), (i-1)*elsize(a))
end
ccall(:jl_array_del_beg, Void, (Any, UInt), a, delta)
return a
end

function _deleteat_end!(a::Vector, i::Integer, delta::Integer)
n = length(a)
if n >= i+delta
ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, Csize_t),
pointer(a, i), pointer(a, i+delta), (n-i-delta+1)*elsize(a))
end
ccall(:jl_array_del_end, Void, (Any, UInt), a, delta)
return a
end
_deleteat!(a::Vector, i::Integer, delta::Integer) =
ccall(:jl_array_del_at, Void, (Any, Int, UInt), a, i - 1, delta)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this still return a ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an internal function so it doesn't have to.


## Dequeue functionality ##

Expand Down Expand Up @@ -528,34 +472,20 @@ function shift!(a::Vector)
end

function insert!{T}(a::Array{T,1}, i::Integer, item)
if !(1 <= i <= length(a)+1)
throw(BoundsError())
end
if i == length(a)+1
return push!(a, item)
end
item = convert(T, item)
# Throw convert error before changing the shape of the array
_item = convert(T, item)
_growat!(a, i, 1)
a[i] = item
# _growat! already did bound check
@inbounds a[i] = _item
return a
end

function deleteat!(a::Vector, i::Integer)
if !(1 <= i <= length(a))
throw(BoundsError())
end
return _deleteat!(a, i, 1)
end
deleteat!(a::Vector, i::Integer) = (_deleteat!(a, i, 1); a)

function deleteat!{T<:Integer}(a::Vector, r::UnitRange{T})
n = length(a)
isempty(r) && return a
f = first(r)
l = last(r)
if !(1 <= f && l <= n)
throw(BoundsError())
end
return _deleteat!(a, f, length(r))
isempty(r) || _deleteat!(a, first(r), length(r))
return a
end

function deleteat!(a::Vector, inds)
Expand Down Expand Up @@ -622,18 +552,9 @@ function splice!{T<:Integer}(a::Vector, r::UnitRange{T}, ins=_default_splice)

if m < d
delta = d - m
if f-1 < n-l
_deleteat_beg!(a, f, delta)
else
_deleteat_end!(a, l-delta+1, delta)
end
_deleteat!(a, (f - 1 < n - l) ? f : (l - delta + 1), delta)
elseif m > d
delta = m - d
if f-1 < n-l
_growat_beg!(a, f, delta)
else
_growat_end!(a, l+1, delta)
end
_growat!(a, (f - 1 < n - l) ? f : (l + 1), m - d)
end

k = 1
Expand Down Expand Up @@ -693,17 +614,22 @@ function vcat{T}(arrays::Vector{T}...)
end
arr = Array{T}(n)
ptr = pointer(arr)
offset = 0
if isbits(T)
elsz = sizeof(T)
elsz = Core.sizeof(T)
else
elsz = Core.sizeof(Ptr{Void})
end
for a in arrays
nba = length(a)*elsz
ccall(:memcpy, Ptr{Void}, (Ptr{Void}, Ptr{Void}, UInt),
ptr+offset, a, nba)
offset += nba
na = length(a)
nba = na * elsz
if isbits(T)
ccall(:memcpy, Ptr{Void}, (Ptr{Void}, Ptr{Void}, UInt),
ptr, a, nba)
else
ccall(:jl_array_ptr_copy, Void, (Any, Ptr{Void}, Any, Ptr{Void}, Int),
arr, ptr, a, pointer(a), na)
end
ptr += nba
end
return arr
end
Expand Down
22 changes: 9 additions & 13 deletions base/c.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,6 @@ if !is_windows()
end
end

# C NUL-terminated string pointers; these can be used in ccall
# instead of Ptr{Cchar} and Ptr{Cwchar_t}, respectively, to enforce
# a check for embedded NUL chars in the string (to avoid silent truncation).
if Int === Int64
bitstype 64 Cstring
bitstype 64 Cwstring
else
bitstype 32 Cstring
bitstype 32 Cwstring
end

# construction from typed pointers
convert{T<:Union{Int8,UInt8}}(::Type{Cstring}, p::Ptr{T}) = box(Cstring, p)
convert(::Type{Cwstring}, p::Ptr{Cwchar_t}) = box(Cwstring, p)
Expand All @@ -80,20 +69,27 @@ unsafe_wrap(::Type{String}, p::Cstring, len::Integer, own::Bool=false) =
unsafe_string(s::Cstring) = unsafe_string(convert(Ptr{UInt8}, s))

# convert strings to String etc. to pass as pointers
cconvert(::Type{Cstring}, s::AbstractString) = String(s)
cconvert(::Type{Cstring}, s::String) =
ccall(:jl_array_cconvert_cstring, Ref{Vector{UInt8}},
(Vector{UInt8},), s.data)
cconvert(::Type{Cstring}, s::AbstractString) =
cconvert(Cstring, String(s)::String)

function cconvert(::Type{Cwstring}, s::AbstractString)
v = transcode(Cwchar_t, String(s).data)
!isempty(v) && v[end] == 0 || push!(v, 0)
return v
end

eltype(::Type{Cstring}) = UInt8
eltype(::Type{Cwstring}) = Cwchar_t

containsnul(p::Ptr, len) =
C_NULL != ccall(:memchr, Ptr{Cchar}, (Ptr{Cchar}, Cint, Csize_t), p, 0, len)
containsnul(s::String) = containsnul(unsafe_convert(Ptr{Cchar}, s), sizeof(s))
containsnul(s::AbstractString) = '\0' in s

function unsafe_convert(::Type{Cstring}, s::String)
function unsafe_convert(::Type{Cstring}, s::Vector{UInt8})
p = unsafe_convert(Ptr{Cchar}, s)
containsnul(p, sizeof(s)) &&
throw(ArgumentError("embedded NULs are not allowed in C strings: $(repr(s))"))
Expand Down
2 changes: 1 addition & 1 deletion base/libgit2/libgit2.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ function clone{P<:AbstractPayload}(repo_url::AbstractString, repo_path::Abstract
remote_cb::Ptr{Void} = C_NULL,
payload::Nullable{P}=Nullable{AbstractPayload}())
# setup clone options
lbranch = Base.cconvert(Cstring, String(branch))
lbranch = Base.cconvert(Cstring, branch)
fetch_opts=FetchOptions(callbacks = RemoteCallbacks(credentials_cb(), payload))
clone_opts = CloneOptions(
bare = Cint(isbare),
Expand Down
4 changes: 2 additions & 2 deletions base/libgit2/oid.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function Oid(id::AbstractString)
(Ptr{Oid}, Ptr{UInt8}, Csize_t), oid_ptr, bstr, len)
else
ccall((:git_oid_fromstrp, :libgit2), Cint,
(Ptr{Oid}, Ptr{Cchar}), oid_ptr, bstr)
(Ptr{Oid}, Cstring), oid_ptr, bstr)
end
err != 0 && return Oid()
return oid_ptr[]
Expand All @@ -46,7 +46,7 @@ function Oid(repo::GitRepo, ref_name::AbstractString)
isempty(repo) && return Oid()
oid_ptr = Ref(Oid())
@check ccall((:git_reference_name_to_id, :libgit2), Cint,
(Ptr{Oid}, Ptr{Void}, Ptr{UInt8}),
(Ptr{Oid}, Ptr{Void}, Cstring),
oid_ptr, repo.ptr, ref_name)
return oid_ptr[]
end
Expand Down
6 changes: 5 additions & 1 deletion base/libgit2/strarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

function StrArrayStruct{T<:AbstractString}(strs::T...)
strcount = length(strs)
map(s->Base.unsafe_convert(Cstring, String(s)), strs) # check for null-strings
for s in strs
if Base.containsnul(s)
throw("embedded NULs are not allowed in C strings: $(repr(s))")
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on line 13, in_ptr is missing a gc-root (although wasn't your fault)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I'm pretty sure libgit2 is full of these though..... a few type declarations make it almost impossible to implement the right rooting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be the only place that pointer is called

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there's a few more places calling unsafe_convert. Anyway, probably better to report/fix separately.....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the one you just fixed, and also one more in clone in libgit2.jl, which isn't great but maybe isn't completely broken either (for now).

sa_strings = convert(Ptr{Cstring}, Libc.malloc(sizeof(Cstring) * strcount))
for i=1:strcount
len = length(strs[i])
Expand Down
6 changes: 3 additions & 3 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ function _require_from_serialized(node::Int, mod::Symbol, path_to_try::String, t
end
elseif node == myid()
myid() == 1 && recompile_stale(mod, path_to_try)
restored = ccall(:jl_restore_incremental, Any, (Ptr{UInt8},), path_to_try)
restored = ccall(:jl_restore_incremental, Any, (Cstring,), path_to_try)
else
content = remotecall_fetch(open, node, read, path_to_try)
restored = _include_from_serialized(content)
Expand Down Expand Up @@ -378,8 +378,8 @@ end
# remote/parallel load

include_string(txt::String, fname::String) =
ccall(:jl_load_file_string, Any, (Ptr{UInt8},Csize_t,Ptr{UInt8},Csize_t),
txt, sizeof(txt), fname, sizeof(fname))
ccall(:jl_load_file_string, Any, (Ptr{UInt8},Csize_t,Cstring),
txt, sizeof(txt), fname)

include_string(txt::AbstractString, fname::AbstractString="string") =
include_string(String(txt), String(fname))
Expand Down
5 changes: 1 addition & 4 deletions base/precompile.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,7 @@ precompile(Libc.TmStruct, (Float64,))
precompile(Base.VersionNumber, (Int, Int, Int, Tuple{}, Tuple{String}))
precompile(Base._atexit, ())
precompile(Base._deleteat!, (Array{UInt8, 1}, Int, Int))
precompile(Base._deleteat_beg!, (Array{UInt8, 1}, Int, Int))
precompile(Base._deleteat_end!, (Array{UInt8, 1}, Int, Int))
precompile(Base._growat_beg!, (Array{UInt8, 1}, Int, Int))
precompile(Base._growat_end!, (Array{UInt8, 1}, Int, Int))
precompile(Base._growat!, (Array{UInt8, 1}, Int, Int))
precompile(Base._setindex!, (Base.Dict{Symbol, Any}, Base.LineEdit.Prompt, Symbol, Int))
precompile(Base._setindex!, (Dict{Any, Any}, Base.LineEdit.PromptState, Base.LineEdit.Prompt, Int))
precompile(Base._setindex!, (Dict{Any, Any}, Bool, WeakRef, Int))
Expand Down
4 changes: 2 additions & 2 deletions base/process.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,8 @@ function _jl_spawn(cmd, argv, loop::Ptr{Void}, pp::Process,
proc = Libc.malloc(_sizeof_uv_process)
disassociate_julia_struct(proc)
error = ccall(:jl_spawn, Int32,
(Ptr{UInt8}, Ptr{Ptr{UInt8}}, Ptr{Void}, Ptr{Void}, Any, Int32,
Ptr{Void}, Int32, Ptr{Void}, Int32, Ptr{Void}, Int32, Ptr{Ptr{UInt8}}, Ptr{UInt8}, Ptr{Void}),
(Cstring, Ptr{Cstring}, Ptr{Void}, Ptr{Void}, Any, Int32,
Ptr{Void}, Int32, Ptr{Void}, Int32, Ptr{Void}, Int32, Ptr{Cstring}, Cstring, Ptr{Void}),
cmd, argv, loop, proc, pp, uvtype(in),
uvhandle(in), uvtype(out), uvhandle(out), uvtype(err), uvhandle(err),
pp.cmd.flags, pp.cmd.env === nothing ? C_NULL : pp.cmd.env, isempty(pp.cmd.dir) ? C_NULL : pp.cmd.dir,
Expand Down
17 changes: 14 additions & 3 deletions base/refpointer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

# C NUL-terminated string pointers; these can be used in ccall
# instead of Ptr{Cchar} and Ptr{Cwchar_t}, respectively, to enforce
# a check for embedded NUL chars in the string (to avoid silent truncation).
if Int === Int64
bitstype 64 Cstring
bitstype 64 Cwstring
else
bitstype 32 Cstring
bitstype 32 Cwstring
end

### General Methods for Ref{T} type

eltype{T}(x::Type{Ref{T}}) = T
Expand Down Expand Up @@ -62,10 +73,10 @@ end
unsafe_convert{T}(::Type{Ptr{Void}}, b::RefArray{T}) = convert(Ptr{Void}, unsafe_convert(Ptr{T}, b))

# convert Arrays to pointer arrays for ccall
function (::Type{Ref{P}}){P<:Ptr,T<:Ptr}(a::Array{T}) # Ref{P<:Ptr}(a::Array{T<:Ptr})
function (::Type{Ref{P}}){P<:Union{Ptr,Cwstring,Cstring},T<:Union{Ptr,Cwstring,Cstring}}(a::Array{T}) # Ref{P<:Ptr}(a::Array{T<:Ptr})
return RefArray(a) # effectively a no-op
end
function (::Type{Ref{P}}){P<:Ptr,T}(a::Array{T}) # Ref{P<:Ptr}(a::Array)
function (::Type{Ref{P}}){P<:Union{Ptr,Cwstring,Cstring},T}(a::Array{T}) # Ref{P<:Ptr}(a::Array)
if (!isbits(T) && T <: eltype(P))
# this Array already has the right memory layout for the requested Ref
return RefArray(a,1,false) # root something, so that this function is type-stable
Expand All @@ -82,7 +93,7 @@ function (::Type{Ref{P}}){P<:Ptr,T}(a::Array{T}) # Ref{P<:Ptr}(a::Array)
end
end
cconvert{P<:Ptr,T<:Ptr}(::Union{Type{Ptr{P}},Type{Ref{P}}}, a::Array{T}) = a
cconvert{P<:Ptr}(::Union{Type{Ptr{P}},Type{Ref{P}}}, a::Array) = Ref{P}(a)
cconvert{P<:Union{Ptr,Cwstring,Cstring}}(::Union{Type{Ptr{P}},Type{Ref{P}}}, a::Array) = Ref{P}(a)

###

Expand Down
Loading