BitArray performance enhancements via introduction of UnsafeArray and UnsafeBitArray

[carlobaldassi]

(See also #2360)

This is possibly controversial, hence the pull request.
First, see the performance comparisons of affected functions:
https://gist.github.com/carlobaldassi/5691454

Some functions become 10 to 20 times faster; for most the boost is about 1.5x / 2x.

Basically, I introduced two new immutable types, UnsafeArray and UnsafeBitArray, for internal use, and used them throughout bitarray.jl. They actually have the semantics of (and inherit from) an AbstractVector, but I see them as wrappers around arrays. The code for UnsafeArray is a few lines of code in array.jl, and could be used there with analogous performance boosts, but I did not touch Array functions in this commit.
What I like here is that if you know what you're doing the code mostly stays clean and readable, basically hiding away stuff like getindex_unchecked(B.chunks, i) and unsafe_store!(pointer(A), x, i) and getting back standard notation as B[i] and A[i] = x.
(This was always the case, with the exception of the find function, for which the pointer notation still gives a better performance, at least in my tests.)
Thoughts?

[StefanKarpinski]

This is pretty interesting. I like the approach of using a differently type object accessing the same memory approach (this seems to be emerging as a very Julian pattern), although it feels a bit odd to wrap the safe version to get the unsafe version – it feels like it should be the other way.

[carlobaldassi]

Yes, wrapper is not really the term here. It's more like the opposite, in that these things extract some minimal amount of information discarding everything else. Maybe a better description would be "unsafe array accessor", or "unsafe array view"?

[JeffBezanson]

This is good work, but I would not merge this. People will get in the habit of using UnsafeArray "for performance", and soon we will have code crashing (or worse, overwriting random data) all over the place. We must not give up safety and elegance.

I notice that some of the changes (e.g. in find) introduce optimizations other than UnsafeArray (the 1:64 inner bit loop). It would be great to have those in a separate PR.

I should really get around to adding the inbounds macro; that is far better than this approach since it cannot spread generically through the system. An UnsafeArray, on the other hand, can end up in any function, including functions that might have out-of-bounds bugs, or that were written intentionally with the idea of giving bounds errors (may be bad style, but that's a separate issue).

As I said in #3224, manually keeping an object alive is much harder to pull off than a local unsafe_load or inbounds, because it is non-local. It is not composable; if a function decides to return an UnsafeArray, that introduces requirements on client code that may have no idea what's going on.

Another good alternative is to use vector performance kernels like memset and memcpy. For example, BitArray fill! could use Array's fill!, which uses memset. Adding new kernels is also acceptable --- they can be written in C or in julia with unsafe_load etc. Vectorized ~ is one possible example. These are good because they provide clear self-contained abstractions; it's easy to verify that they work, and it's easy to verify that you've passed the right arguments, and then you're done.

[carlobaldassi]

Yes, I see the danger (even though these types are of course not exported), and I agree that the need to keep the array alive is the biggest issue. One possibility to mitigate the problem of people inadvertently using unsafe arrays and passing them around would be not making them be AbstractArrays (the code in the PR only misses endof and isempty implementations, which are trivial). This would essentially only leave open ::Any function signatures, preventing most unwanted propagation.
I guess an inbounds macro would be better though, if it can achieve the same performance (it's not clear to me how that would be implemented, and how good would the compiler be at optimizing the resulting code - with UnsafeArrays it's pretty good).
BTW my starting point was having written everything with unsafe_load and unsafe_store!, and getting fast but messy code. I still have that version, of course: would that be ok? Or is it better to wait for an inbounds macro?

(also: I thought about using Array's fill!, but that gives an advantage only for filling with zeros at the moment)

[JeffBezanson]

I really think we should avoid unsafe or very messy code. To me those are not worth 2x, maybe even 4x performance. These things should be solved in the compiler.

Another tricky thing I'm not yet doing in the compiler is hoisting access to the data pointer of Vectors, since they can grow and therefore move at almost any point. This also makes it hard to do the optimization correctly manually using UnsafeArray, yet another reason not to use it. But I can improve this situation in the compiler, though of course sometimes it will have to be conservative and do some extra loads.

[carlobaldassi]

Even though I am not convinced myself that this scheme is fine, I'll still try to further expand of the previous argument, for the sake of discussion: it might be argued that if we don't have UnsafeArray <: AbstractVector, it's basically as dangerous as Ptr.
The biggest difference I see is that Ptr and related functions are basically only used in interfacing with libraries, where you're leaving Julia and non-safety is implicit; on the other hand, UnsafeArrays would 1) not be exported 2) not be documented except for a warning in the code comments 3) have an extremely limited set of applicable methods 4) have the explicit "Unsafe" prefix, which should at least ring a bell to anyone venturing to use it 5) ideally, only be used in kernels within base, as in:

Adding new kernels is also acceptable --- they can be written in C or in julia with unsafe_load etc. Vectorized ~ is one possible example. These are good because they provide clear self-contained abstractions; it's easy to verify that they work, and it's easy to verify that you've passed the right arguments, and then you're done.

It's basically just a matter of substituting unsafe_load with unsafe in the above sentence.

(That said, it's totally fine with me if this does not get in.)

[JeffBezanson]

No, UnsafeArray is utterly different than unsafe_load, or calling memcpy, etc. It acts as a "poison pill" that renders unsafe all code that it touches. The problem is that it introduces unsafe behavior into a generic context, instead of into a specific context. If I call somebody's big library with f(unsafe(a)) instead of f(a), it could amount to wrapping all of their code in inbounds, which is an overly-bold declaration to make. As soon as you enter b=unsafe(a) and start doing a few things with b, you can cause rampant destruction.

[coveralls]

Changes Unknown when pulling 90b1a46 on carlobaldassi:bitperf2 into * on JuliaLang:master*.

[tkelman]

wtf, mystery coveralls again? looks like some repo with a bunch of php - they definitely have a bug somewhere we should probably report to someone