RFC : Implementation of rol! and ror!

[amitjamadagni]

I am new to the community. I have worked on the addition of rol! and ror!. It would be really helpful to hear some comments from the community. Thanks.

[ViralBShah]

Cc: @carlobaldassi

[carlobaldassi]

These definitions do the job, however the usual definitions of in-place mutating operators take advantage of the fact that you can use them to avoid allocating memory.

The versions as written in this PR instead offer a very small advantage over writing A = rol(A, i), since they basically are syntactic sugar for that.

So I think we'd rather have functions rol!(dest::BitVector, src::BitVector, i::Integer) which use a single UInt64 buffer to do their job.

However, out of curiosity: @amitjamadagni, do you have a need for these functions? I don't think I've ever seen them come up in code, and they are not defined for anything but BitArrays.

(A minor comment on the code is that the copy_chunks! calls can become simply B.chunks = A.chunks, which is simpler and faster, although it makes it even more clear that there's wasted allocation there.)

In any case, tests and documentation are also always needed whenever adding functionality.

[amitjamadagni]

@carlobaldassi I had no direct requirement of function. I was going through some of the TODO tasks and this was one of them.
I thought retaining the parameters of ror, rol for ror!, rol! was more relevant. I do get the point that is extra usage of memory. Anyways please do let me know about the format of such functions. I would add the tests and documentation as soon as possible. Thanks.

[carlobaldassi]

Upon thinking about it a little more, I think we'd need 2 signatures. The first one would be the in-place one, rol!(B::BitArray, i::Integer).

The second one would be rol!(dest::BitVector, src::BitVector, i::Integer) and would be implemented in such a way that dest = rol(src, i), except that dest would not be a newly allocated BitArray but rather an existing one, just like e.g. copy!(dest::BitArray, src::BitArray) vs dest = copy(src::BitArray). The length of dest and src should be the same. The code would check if dest === src, and call the in-place method in such case; otherwise it could reuse the same code which we have now.

The current rol and ror could then be reimplemented in terms of the new mutating versions, i.e. rol(B, i) via rol!(similar(B), B, i) etc.

Actually, one potential problem with this is that even if dest !== src, they could still be aliasing the same underlying memory, or overlapping portions (e.g. via mmapping). I'm not sure if we can deal with that.

Also, the actual implementation of the in-place version can be quite tricky, as it should cycle through the chunks jumping around and deal with situations of potential overlap. Take the simplest situation, in which we want to rotate a BitArray B, whose length is a multiple of 64, by i=64n bits with n != 0; then we should do something like:

Bc = B.chunks
lBc = length(B.chunks)
for j0 = 1:(lBc % n == 0 ? div(lBc, n) - 1 : 1)
    j = j0
    x = Bc[j]
    while true
        k = mod(j - n - 1, length(Bc)) + 1
        y = Bc[k]
        Bc[k] = x
        x, j = y, k
        j == j0 && break
    end
end

i.e. cover the whole array with cycles of step n (we need div(lBc,n)-1 such cycles if lBc is divisible by n, just one otherwise).

The general case where the rotation parameter and the length are not a multiple of 64 is more complicated and should use copy_chunks! probably, using a couple of auxiliary chunk vectors of length 1 for x and y (see e.g. the reverse!(::BitVector) function) and paying attention at what happens at the end of the BitArray.

(note I didn't test the above code)

[amitjamadagni]

@carlobaldassi Thanks for the inputs on this. I will work on it and send in some commits with my understanding.

[amitjamadagni]

@carlobaldassi Here is an implementation I have attempted. Please do let me know your thoughts on it.
https://gist.github.com/amitjamadagni/b27ecc63f043b40678f4
The case of memory overlap, I am still thinking through and hopefully I can make some progress at it.

[carlobaldassi]

The ror! method should check that length(dest) == length(src).
Some simpler logic could be used in the branching, i.e. something like Bc == (src === dest ? copy(src.chunks) : src.chunks) and then you'd call the copy_chunks! methods with Bc in the third argument instead of src.chunks or Bc.chunks which you have now.

What is this about:

#edge-case failure if B is all false in the case of ror.

?

[amitjamadagni]

@carlobaldassi thanks. I will work on the inputs. My apologies for the comment. I had a doubt but it stands clarified.

[amitjamadagni]

@carlobaldassi I have been looking at the failing tests. Should they be based on the idea of how splice! is being tested ??

[carlobaldassi]

The check should throw an error in case it's false, rather then continuing silently

[carlobaldassi]

It's more readable if you write it as length(dest) == length(src) || trhrow(...) and then de-indent the following lines.

[amitjamadagni]

@carlobaldassi could you please review this. The test cases seem to work. Thanks.

[carlobaldassi]

This is not very clear, even if t's self-evident from the argument names the description should state that the rotation is performed on src and put into dest.

Also a very minor nitpick: please put a space between the function arguments after the commas - here and below.

[carlobaldassi]

@amitjamadagni
I added a couple of minor comments. Besides that, the new functionality should include specific tests, e.g. application in-place to some BitVector etc.

Are you planning to do an attempt at the non-copying in-place version? If not, just add a TODO note in a comment and I'll probably try that when I have some time, so we can merge this in the meanwhile. Otherwise we can wait.

[amitjamadagni]

@carlobaldassi seems a time out on the build, therefore failing. Anyways a review on this would be helpful.

[amitjamadagni]

@carlobaldassi done.

[ViralBShah]

It would be nice to squash all the commits.

[amitjamadagni]

@ViralBShah done.

[amitjamadagni]

@carlobaldassi : any comments on the commit would be really helpful.

[carlobaldassi]

Sorry for the delay, I've been away with no internet connection.

[amitjamadagni]

@carlobaldassi : Thanks for all the help and also for the merge.

[stevengj]

I just noticed these functions. As @carlobaldassi mentioned, it would be better if these truly operated in-place for src === dest, in which case you could have a single-argument version of the function.

I don't know if you are aware of this, but there is actually a relatively simple trick (which I learned from @matteo-frigo) for cyclically shifting an array A (in this case the chunks array) by an arbitrary amount 0 < i < length(A):

• First: reverse!(A, 1, i) and reverse!(A, i+1, length(A)).
• Second: reverse!(A)

This requires two passes over the array. Theoretically, you can do it in a single pass by chasing the cycles of the permutation, but in practice the two-pass trick is far more efficient, both because chasing the cycles involves a lot of auxiliary computation and because reverse! has much better spatial locality (hence better cache-line utilization).

Might be nice to apply this trick define ror!(a) and rol!(a) functions.

[stevengj]

(Similarly, one could define circshift! as an in-place version of our current circshift function.)

[matteo-frigo]

(See also Knuth Vol. 1 3rd ed., Section 1.3.3 Exercises 34, 35, and 36. The trick is in exercise 36.)

[carlobaldassi]

@stevengj good idea, I'll get to it soon-ish (if no one beats me to it).