Array resizing fixes and clean up #16893

yuyichao · 2016-06-12T13:18:55Z

Move memcpy and memmove from julia to C (this is dangerous especially
for ptrarray)
Make sure resizing a ptrarray always have the new memory cleared
Make sure resizing a byte array always have the implicit NUL byte
Use array flag to conditionally copy the array when there's no implicit NUL
byte in the array.
Add more test for shared array resizing and implicit NUL byte check

Started as removing unsafe memcpy/memmove from julia files and make sure array resizing clear the memory if necessary and fix the implicit NUL byte and #16499 along the way

timholy · 2016-06-12T13:32:42Z

It's a little sad to see more stuff move into C, though of course a lot of that code was basically C in disguise (lots of ccalls). Out of curiosity, why are the direct ccalls to memmove from julia problematic?

yuyichao · 2016-06-12T13:36:26Z

The memmove is currently safe, but some of the code that uses it can cause unexpected side effect (e.g. when resizing a reinterpreted array, see test). This clean up is split out from my WIP for array card marking and with that, memmove in the array will be unsafe since it could move reference across multiple card.

The memcpy is the really unsafe one, they can miss write barrier.

yuyichao · 2016-06-12T13:39:04Z

And the write barrier concern above only applies to pointer array, it is perfectly safe to use memcpy/memmove on bits arrays (note the unsafe_copy I didn't remove in array.jl). The ones I moved to C can all be called on pointer arrays.

tkelman · 2016-06-12T17:47:56Z

base/array.jl

-    return a
-end
+_deleteat!(a::Vector, i::Integer, delta::Integer) =
+    ccall(:jl_array_del_at, Void, (Any, Int, UInt), a, i - 1, delta)


should this still return a ?

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

tkelman · 2016-06-12T17:57:14Z

src/array.c

        }
-        memcpy(newdata + offsnb, (char*)a->data, oldnbytes);
    }
+    (void)oldlen;


what does this accomplish?

Suppress compiler warning with assertion off.

vtjnash · 2016-06-13T15:40:32Z

base/c.jl

+cconvert(::Type{Cstring}, s::String) =
+    ccall(:jl_array_cconvert_cstring, Ref{Vector{UInt8}},
+          (Vector{UInt8},), s.data)
+cconvert(::Type{Cstring}, s::AbstractString) = String(s)::String


= cconvert(Cstring, String(s)::String)?

Sure, I guess custom string type could use the unsafe constructor.....

JeffBezanson · 2016-06-13T18:16:47Z

-1

Moving the code to C does not make it safer. We just need the appropriate checks to only use memcpy/memmove when safe. I think this is too much code churn.

I thought we were moving away from the guaranteed 0 byte at the end of byte arrays?

The direction we want to go is to make the built-in type a simpler buffer type, and move more of the array code to Julia.

yuyichao · 2016-06-13T18:24:41Z

Moving the code to C does not make it safer.

Move code to C does make it safer since we can guarantee that there's no allocation in between and the array cannot get old because of an unexpected allocation. Another way to solve this is to not use memcpy or memmove at all for pointer arrays and take the huge performance hit there (due to the write barrier, the loop can't be vectorized).

I thought we were moving away from the guaranteed 0 byte at the end of byte arrays?

That means we'll have to do a copy every time someone does a ccall with Cstring on a String. I don't think that regression is acceptable.

The direction we want to go is to make the built-in type a simpler buffer type, and move more of the array code to Julia.

I don't really think that's the direction either. The buffer type needs to be strongly typed or it can't be used in julia code safely. The buffer type will also kill the inline data optimization for small arrays. The more regression on array we want to avoid, the more we'll need features from the current array implementation and the closer the new buffer type to the current array. I don't really see any problem of using the array type as a buffer. I agree that there are certain features that are useful in other types too (c99 va array member for example) but replace the array implementation with a different one and take the performance hit doesn't feel like the right way to go.

JeffBezanson · 2016-06-13T18:25:08Z

src/array.c

+    return new_ary;
+}
+
+JL_DLLEXPORT jl_array_t *jl_array_vcat_vectors(jl_value_t *arrays)


This is too high-level a function to put in the runtime. It also might be slower in some cases, since it requires allocating a tuple for the arguments, while the julia version can leave the arguments on the stack.

Yes, this should be the only case that there can be (one) more allocation compare to the original version. The allocation of a tuple is much cheaper than an array though.

Can we use the copy!/unsafe_copy! API more here? We can use repeated calls to that to implement vcat in julia, and we can use it to implement copy as well. unsafe_copy! is a low-level enough function to move to C if necessary, but it doesn't seem necessary fortunately.

Not really for vcat and copy. The problem is not about the memcpy/memmove functions itself but what we do before it. It is not safe to use unsafe_copy! on a pointer array as long as we have any allocation after the array itself is allocated.

I agree that having a gc-unsafe region concept in julia is useful. However, for it to be remotely useful, we need to be able to statically analyze what can allocate and what can't (also what can iteract with GC and what can't). The vcat and copy case can also be more tricky since we will need a way to express that no allocation or safe points are allowed when we return from jl_new_array but we don't want to disable the GC before calling jl_new_array (or Array{T}(n)) (the C code is written to make sure the array is young when jl_new_array returns).

would it be that expensive to simply dynamically check that it is still young (almost all cases) ? basically exactly what gc_wb_back does anyway

Sure, but isnt what's missing just a couple manual wb_back/wb_fwd intrinsics ? That's a fundamental basic block that I don't think anybody would object to it being implemented in codegen. I'm actually surprised we never needed those.

We need to have some function like copy! that is safe to use, and can be used to implement vcat. It's a very useful function, since it supports destination and source offsets (and could support strides as well if needed). unsafe_copy! already does an isbits check, so it should be ok to use. The extra tuple allocation is also unacceptable; we have too many performance regressions as it is. So please move vcat back to julia and just use unsafe_copy! instead of memcpy.

Sure, but isnt what's missing just a couple manual wb_back/wb_fwd intrinsics

How should it be used?

ah yep, well maybe for this restricted case the easiest way to do so would be a function-wide (meta nogc) that has a short whitelist of whatever is allowed and implement copy! with that. In that case if it's only a ccall to memcpy (or even a bunch of unsafe_load/stores in a loop if llvm does a good enough job) followed by a wb intrinsic the whitelist should be short enough.

Or maybe implement copy! in C but since both those little features seems like something that we could use/extend in the future...

I guess I can implement unsafe_copy! in C. That should solve these issues.

FWIW, using the current unsafe_copy! isn't the right solution since it'll be much more expensive than the current memcpy based implementation for pointer arrays.

When benchmarking this, it seems that the store_unboxed check might have a significant performance effect. I'll try to verify this more carefully and if that is the case I may move some of the similar implementation to C (they call C to allocate the array anyway)

JeffBezanson · 2016-06-13T18:28:25Z

replace the array implementation with a different one and take the performance hit doesn't feel like the right way to go

The goal would be to do it without a performance hit.

yuyichao · 2016-06-13T18:35:35Z

The goal would be to do it without a performance hit.

Sure, there are a few of the optimizations and features that I don't think can be implemented without a lot of the features of the current optimization. (e.g. that high dim arrays can't be resized and unsafe_wrap(Array)).

In any case, that discussion is unrelated to this PR. The functions in this PR will be equally useful on the buffer type if we have that. Assuming we don't want to loose the ability to use memcpy and memmove on ponter arrays.

JeffBezanson · 2016-06-13T18:37:33Z

Can this be handled by manually introducing gc-unsafe regions in the julia code?

yuyichao · 2016-06-13T18:50:43Z

Can this be handled by manually introducing gc-unsafe regions in the julia code?

I believe it's much harder (needs more special cases in the codegen to know what code can or cannot allocate/trigger GC), more likely to have performance regression (we need to disable GC, which is a thread synchronization), and will encourage people to disable GC (since that's how we can implement it).

This also won't help deleteat!, which does the wrong thing for reinterpreted array (it modify the array before unsharing). (Sure, we can always unshare it first, and it will introduce a ~10% performance regression for small arrays due to the additional ccall)

vtjnash · 2016-06-23T04:02:51Z

src/array.c

+}
+
+// Copy element by element until we hit a young object, at which point
+// we can


can what? don't leave me hanging!

continue using memmove.

Apparently got interrupted while writing this comment....

yuyichao · 2016-06-24T15:42:59Z

CI passed a few times. Local GC stress tests passed. Local performance tests checks out.

AFAICT, almost all of the functions touched by this PR should have the same or better performance (especially fast path). The only exception is vcat of pointer arrays, which has a up to 3% slow down for concatenating empty or small arrays due to the write barrier check.

Some additional notes about the implicit NUL byte. The guarantee we had before and relies on is that, a byte array that has only be resized at the end always has the implicit NUL byte. I kept this behavior since it seems to be way too breaking if that is changed. A macro JL_ARRAY_IMPL_NUL is added in array.c to control this.

In the long term, I believe we should at least keep the guarantee to allocate one more implicit byte for the byte array since otherwise the Cstring cconvert has to make a copy most of the time. This also only happens during allocation and resizing of the buffer and not when resizing the array so the performance impact is much smaller.

Another issue about using the Cstring cconvert for everything that need the implicit NUL byte is that the Cstring is also used to make sure there's no embedded NUL byte which is a very expensive check. In many cases, the user can easily know that the string doesn't have embedded NUL and it would be too expensive to do the embedded NUL check just for the terminating NUL byte. I'm thinking maybe we can Ptr{UInt8} on String to only add implicit NUL byte and let Cstring on String to do both?

* Make sure that newly allocated arrays are always young * Micro optimize `sizehint!` * Implement `copy(::Array)` in C to avoid calling `memcpy` that bypasses the write barrier.

* Always call `cconvert` before calling `unsafe_convert`. * Optimize parse functions for non-NUL terminated input. * Use `Cstring` for `ccall`s that's expecting a NUL terminated string

* Use it in `jl_load_` * Remove the length parameter from `jl_load`, `jl_load_file_string` and `jl_parse_eval_all` to NOT pretend they support non-NUL-terminated strings.

Add tests for implicit extra byte check.

* Move some `memcpy` and `memmove` from julia to C This is dangerous especially for `ptrarray` since it bypasses the write barrier. * Make sure resizing a ptrarray always have the new memory cleared * Add more test for shared array resizing

* Use `unsafe_copy!` instead of `memcpy` in `vcat` to avoid bypassing the write barrier. * Add test for `copy!` on `#undef` and `unsafe_copy!` with memory alias.

vtjnash · 2016-06-28T18:56:52Z

Will merge tomorrow if no comments.

tkelman · 2016-06-28T19:02:10Z

test/loading.jl

-
 include("test_sourcepath.jl")
-thefname = "the fname!//\\&\0\1*"
+thefname = "the fname!//\\&\1*"


why is this being removed?

We never actually support filenames (either real or fake ones) that are not NUL-terminated strings. E.g., for real filenames, the jl_stats call assumes NUL termination, for both real and fake ones, every functions that uses jl_filename assumes it is NUL terminated. It might be possible to go through everything and fix them but it would be much more work and I highly doubt supporting embeded NUL or non-NUL terminating strings as filename is useful.

how were we previously passing this test then?

The value was accepted but is basically treated as a C string up to the NUL character. It is never used anywhere or checked against anything. You can see this by doing something that actually uses the filename.

julia> thefname = "the fname!\0and this part is missing" "the fname!\0and this part is missing" julia> include_string("include_string_test() = error()", thefname)() ERROR: in include_string_test() at ./the fname!:1 in eval(::Module, ::Any) at ./boot.jl:234 in macro expansion at ./REPL.jl:92 [inlined] in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

Note that the filename for include_string_test doesn't have anything after the \0 byte.

The commit remove the support for non NUL-terminating string explicitly so passing a malformed string like that throws an error.

JeffBezanson · 2016-06-28T19:32:14Z

lgtm.

tkelman · 2016-07-02T19:30:46Z

src/array.c

+    // No need to explicitly unshare.
+    // Shared arrays are guaranteed to trigger the slow path for growing.
+    size_t n = jl_array_nrows(a);
+    if (idx < 0 || idx > n)


should this be >= ?

No, it is allowed to grow at the end of the array (jl_array_grow_end below also calls jl_array_grow_at_end with inc == n). In general, there are n + 1 positions one can insert elements in an n elements array, which are represented as 0 to n here.

… 0.3

rfourquet · 2017-10-03T09:35:48Z

base/array.jl


 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.


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.

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

tkelman reviewed Jun 12, 2016
View reviewed changes

yuyichao force-pushed the yyc/gc/array branch from be8564c to be49e66 Compare June 12, 2016 17:49

tkelman reviewed Jun 12, 2016
View reviewed changes

yuyichao force-pushed the yyc/gc/array branch 2 times, most recently from ebe9d6a to b5e6f0d Compare June 13, 2016 11:52

vtjnash reviewed Jun 13, 2016
View reviewed changes

JeffBezanson reviewed Jun 13, 2016
View reviewed changes

yuyichao force-pushed the yyc/gc/array branch 8 times, most recently from 14f1ccc to 16fb745 Compare June 14, 2016 15:51

vtjnash reviewed Jun 23, 2016
View reviewed changes

yuyichao force-pushed the yyc/gc/array branch 2 times, most recently from bd535d7 to 23c75fa Compare June 23, 2016 19:50

yuyichao force-pushed the yyc/gc/array branch 3 times, most recently from dcf3413 to d463b8e Compare June 25, 2016 15:01

yuyichao mentioned this pull request Jun 25, 2016

More explicit TLS access and GC allocation optimization #17116

Merged

yuyichao force-pushed the yyc/gc/array branch from d463b8e to 48d0c58 Compare June 26, 2016 16:38

yuyichao added 6 commits June 27, 2016 08:38

Array micro optimizations

16f682c

* Make sure that newly allocated arrays are always young * Micro optimize `sizehint!` * Implement `copy(::Array)` in C to avoid calling `memcpy` that bypasses the write barrier.

A few Cstring related fix

91488ca

* Always call `cconvert` before calling `unsafe_convert`. * Optimize parse functions for non-NUL terminated input. * Use `Cstring` for `ccall`s that's expecting a NUL terminated string

Implement implicit byte checker in C.

5d524ec

* Use it in `jl_load_` * Remove the length parameter from `jl_load`, `jl_load_file_string` and `jl_parse_eval_all` to NOT pretend they support non-NUL-terminated strings.

Implement cconvert(Cstring) in C

cee4cb7

Add tests for implicit extra byte check.

Array resizing fixes and clean up

d23890f

* Move some `memcpy` and `memmove` from julia to C This is dangerous especially for `ptrarray` since it bypasses the write barrier. * Make sure resizing a ptrarray always have the new memory cleared * Add more test for shared array resizing

Implement faster and safer unsafe_copy! in C

dd3ba9b

* Use `unsafe_copy!` instead of `memcpy` in `vcat` to avoid bypassing the write barrier. * Add test for `copy!` on `#undef` and `unsafe_copy!` with memory alias.

yuyichao force-pushed the yyc/gc/array branch from 48d0c58 to dd3ba9b Compare June 27, 2016 12:38

tkelman reviewed Jun 28, 2016
View reviewed changes

yuyichao merged commit 70a7da9 into master Jun 29, 2016

yuyichao deleted the yyc/gc/array branch June 29, 2016 20:00

tkelman reviewed Jul 2, 2016
View reviewed changes

stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Jul 11, 2016

fix for JuliaLang/julia#16893; note that cconvert is not available on…

09e51bc

… 0.3

stevengj mentioned this pull request Jul 11, 2016

fix tests to work with JuliaLang/julia#16590 JuliaLang/Compat.jl#249

Merged

dpsanders pushed a commit to dpsanders/Compat.jl that referenced this pull request Feb 1, 2017

fix for JuliaLang/julia#16893; note that cconvert is not available on…

312ea23

… 0.3

rfourquet reviewed Oct 3, 2017

View reviewed changes

dhoegh mentioned this pull request Nov 6, 2017

Performance regression in deleteat! #24494

Closed

Uh oh!

Array resizing fixes and clean up #16893

Array resizing fixes and clean up #16893

Uh oh!

Conversation

yuyichao commented Jun 12, 2016

Uh oh!

timholy commented Jun 12, 2016

Uh oh!

yuyichao commented Jun 12, 2016

Uh oh!

yuyichao commented Jun 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Jun 13, 2016

Uh oh!

yuyichao commented Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuyichao Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Jun 13, 2016

Uh oh!

yuyichao commented Jun 13, 2016

Uh oh!

JeffBezanson commented Jun 13, 2016

Uh oh!

yuyichao commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuyichao commented Jun 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vtjnash commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Jun 28, 2016

yuyichao commented Jun 13, 2016 •

edited

Loading

JeffBezanson Jun 13, 2016 •

edited

Loading

yuyichao Jun 13, 2016 •

edited

Loading

yuyichao commented Jun 24, 2016 •

edited

Loading

yuyichao Jul 2, 2016 •

edited

Loading