Optimization + inplace operations

[lemire]

Points to consider:

1. I have added inplace operations. These can be very useful, as they can avoid an unnecessary copy. For fun, consider the case where you have two bloom filters, but you want to merge them. A simple in-place union would be best.

2. I suggest being conservative about modifying the object. For example, when calling "Equal", it is kind of surprising that the calling object would be modified. This could lead to evil bugs.

3. For the intersection, union... operations, you had a loop with an embedded if clause. I don't know if the compiler is smart enough to optimize this away. Maybe. Maybe not. I have rewritten the code so that you don't have to rely on having a smart compiler.

4. I have modified the NextSet function is that it is more Go-ish. I don't know whether it is better, faster... I did not benchmark it, but it feels nicer to me.

5. Though I have not fixed it, I think that there is a deeper design issue. My understanding is that we access slices/arrays using an int type index in Go. Slice lengths are similarly int-valued. So you are doing a lot of messing around with uints, but this is not necessary, I think.

[willf]

Thanks, Daniel. I will review.

On Tue, Feb 11, 2014 at 11:13 AM, Daniel Lemire [email protected]:

Points to consider:

1. I have added inplace operations. These can be very useful, as they can
   avoid an unnecessary copy. For fun, consider the case where you have two
   bloom filters, but you want to merge them. A simple in-place union would be
   best.

2. I suggest being conservative about modifying the object. For example,
   when calling "Equal", it is kind of surprising that the calling object
   would be modified. This could lead to evil bugs.

3. For the intersection, union... operations, you had a loop with an
   embedded if clause. I don't know if the compiler is smart enough to
   optimize this away. Maybe. Maybe not. I have rewritten the code so that you
   don't have to rely on having a smart compiler.

4. I have modified the NextSet function is that it is more Go-ish. I don't
   know whether it is better, faster... I did not benchmark it, but it feels
   nicer to me.

5. Though I have not fixed it, I think that there is a deeper design
   issue. My understanding is that we access slices/arrays using an int type
   index in Go. Slice lengths are similarly int-valued. So you are doing a lot

of messing around with uints, but this is not necessary, I think.

You can merge this Pull Request by running

git pull https://github.com/lemire/bitset master

Or view, comment on, or merge it at:

#14
Commit Summary

• Readme should document how to iterate over set bits.
• Merge remote-tracking branch 'upstream/develop'
• Updated the iteration routines
• Now with inplace operations

File Changes

• M README.md https://github.com/willf/bitset/pull/14/files#diff-0(3)
• M bitset.go https://github.com/willf/bitset/pull/14/files#diff-1(172)
• M bitset_test.gohttps://github.com/Optimization + inplace operations #14/files#diff-2(116)

Patch Links:

• https://github.com/willf/bitset/pull/14.patch
• https://github.com/willf/bitset/pull/14.diff

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14
.

Will

[willf]

Thanks, Daniel. I merged these changes into develop and master.

On Tue, Feb 11, 2014 at 11:25 AM, Will Fitzgerald <[email protected]

wrote:

Thanks, Daniel. I will review.

On Tue, Feb 11, 2014 at 11:13 AM, Daniel Lemire [email protected]:

Points to consider:

1. I have added inplace operations. These can be very useful, as they can
   avoid an unnecessary copy. For fun, consider the case where you have two
   bloom filters, but you want to merge them. A simple in-place union would be
   best.

2. I suggest being conservative about modifying the object. For example,
   when calling "Equal", it is kind of surprising that the calling object
   would be modified. This could lead to evil bugs.

3. For the intersection, union... operations, you had a loop with an
   embedded if clause. I don't know if the compiler is smart enough to
   optimize this away. Maybe. Maybe not. I have rewritten the code so that you
   don't have to rely on having a smart compiler.

4. I have modified the NextSet function is that it is more Go-ish. I
   don't know whether it is better, faster... I did not benchmark it, but it
   feels nicer to me.

5. Though I have not fixed it, I think that there is a deeper design
   issue. My understanding is that we access slices/arrays using an int type
   index in Go. Slice lengths are similarly int-valued. So you are doing a lot

of messing around with uints, but this is not necessary, I think.

You can merge this Pull Request by running

git pull https://github.com/lemire/bitset master

Or view, comment on, or merge it at:

#14
Commit Summary

• Readme should document how to iterate over set bits.
• Merge remote-tracking branch 'upstream/develop'
• Updated the iteration routines
• Now with inplace operations

File Changes

• M README.md https://github.com/willf/bitset/pull/14/files#diff-0(3)
• M bitset.go https://github.com/willf/bitset/pull/14/files#diff-1(172)
• M bitset_test.gohttps://github.com/Optimization + inplace operations #14/files#diff-2(116)

Patch Links:

• https://github.com/willf/bitset/pull/14.patch
• https://github.com/willf/bitset/pull/14.diff

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14
.

Will

Will