Support 1.23 iterator in a backward-compatible way

[amikai]

Resolve #471

Provide two new functions:

• Values: An iterator in normal order
• Backward: An iterator in reverse order

Unit test:

• iter123_test.go: Test the 1.23 iterator by for-range
• iter_test.go: Test the iterator by call function way

Design consideration

• I used a naming method similar to slices.Values and slices.Backward to make it easier to use.
• It is compatible to use Values and Backward before version 1.23 without for-range function syntax, which means we don't need to update the go.mod version.
• For version 1.23 or later, we can use the for-range function syntax, and these two functions are also compatible with the iterator type in the iter package.

Hi @lemire,
The unit test I wrote uses a similar approach to the one for testing Bitmap.ReverseIterator and Bitmap.Iterator. However, I am not confident that the test case is robust enough. Could you please review it? If you find the implementation acceptable, I will proceed with writing the Go doc comments and add examples to it.

[lemire]

Can you add benchmarks ? See

roaring/benchmark_test.go

Line 18 in fb67118

func BenchmarkIteratorAlloc(b *testing.B) {

[amikai]

Hi @lemire
The benchmark is added in df9d672

[lemire]

@amikai Can you share your benchmark results ? E.g., on your own machine ?

[lemire]

An int might be a signed 32-bit integer, so MaxUint32 can't be represented.

But when it works, you might be allocating 512 MB of memory. Please don't do this as part of our routine tests.

[lemire]

This will allocate hundreds of megabytes. Please don't do this in tests.

[amikai]

In fact, I just wrote a similar test of the TestReverseIterator to make sure the behavior is same as before. How about I change it to values := []uint32{0, 2, 15, 16, 31, 32, 33, 9999, MaxUint16}? Is that okay with you?

roaring/roaring_test.go

Lines 2428 to 2449 in b705c37

	t.Run("#1", func(t *testing.T) {
	values := []uint32{0, 2, 15, 16, 31, 32, 33, 9999, MaxUint16, MaxUint32}
	bm := New()
	for n := 0; n < len(values); n++ {
	bm.Add(values[n])
	}
	i := bm.ReverseIterator()
	n := len(values) - 1
	for i.HasNext() {
	assert.EqualValues(t, i.Next(), values[n])
	n--
	}
	// HasNext() was terminating early - add test
	i = bm.ReverseIterator()
	n = len(values) - 1
	for ; n >= 0; n-- {
	assert.EqualValues(t, i.Next(), values[n])
	assert.False(t, n > 0 && !i.HasNext())
	}
	})

[lemire]

Good.

[lemire]

This will allocate hundreds of megabytes. Please don't do this in tests.

[amikai]

Same as before, I just wrote a similar test to make sure the behavior same as before. Will delete this test.

roaring/roaring_test.go

Lines 2488 to 2496 in b705c37

	t.Run("#6", func(t *testing.T) {
	bm := New()
	bm.Add(MaxUint32)
	i := bm.ReverseIterator()
	assert.True(t, i.HasNext())
	assert.EqualValues(t, uint32(MaxUint32), i.Next())
	assert.False(t, i.HasNext())
	})

[lemire]

@amikai It is entirely possible that it was pre-existing. it is still a terrible idea. :-)

[lemire]

To be clear, we do not have AddInt(MaxUint32) in the code base because that does not work....

MaxUint32 may not fit in an int.

[amikai]

Sorry, I didn't see clearly.

[amikai]

Hi @lemire, here is the benchmark on my computer:

goos: darwin
goarch: arm64
pkg: github.com/RoaringBitmap/roaring/v2
cpu: Apple M2
BenchmarkIterator123/simple_iteration_with_alloc-8                 24210             48902 ns/op             112 B/op          1 allocs/op
BenchmarkIterator123/simple_iteration-8                            25024             47947 ns/op               0 B/op          0 allocs/op
BenchmarkIterator123/values_iteration-8                            23919             49615 ns/op             112 B/op          1 allocs/op
BenchmarkIterator123/reverse_iteration_with_alloc-8                22402             54250 ns/op             112 B/op          1 allocs/op
BenchmarkIterator123/reverse_iteration-8                           25252             47785 ns/op               0 B/op          0 allocs/op
BenchmarkIterator123/backward_iteration-8                          22621             52362 ns/op             112 B/op          1 allocs/op
BenchmarkIterator123/many_iteration_with_alloc-8                   57945             20636 ns/op            4224 B/op          2 allocs/op
BenchmarkIterator123/many_iteration-8                              59941             20383 ns/op               0 B/op          0 allocs/op
BenchmarkIterator123/values_iteration_1.23-8                       24662             49834 ns/op             112 B/op          1 allocs/op
BenchmarkIterator123/backward_iteration_1.23-8                     23325             51611 ns/op             112 B/op          1 allocs/op
BenchmarkIteratorAlloc/simple_iteration_with_alloc-8               24333             48729 ns/op             112 B/op          1 allocs/op
BenchmarkIteratorAlloc/simple_iteration-8                          24201             50280 ns/op               0 B/op          0 allocs/op
BenchmarkIteratorAlloc/values_iteration-8                          24680             49221 ns/op             112 B/op          1 allocs/op
BenchmarkIteratorAlloc/reverse_iteration_with_alloc-8              23397             51549 ns/op             112 B/op          1 allocs/op
BenchmarkIteratorAlloc/reverse_iteration-8                         26040             45923 ns/op               0 B/op          0 allocs/op
BenchmarkIteratorAlloc/backward_iteration-8                        23385             51324 ns/op             112 B/op          1 allocs/op
BenchmarkIteratorAlloc/many_iteration_with_alloc-8                 57744             20754 ns/op            4224 B/op          2 allocs/op
BenchmarkIteratorAlloc/many_iteration-8                            57397             19987 ns/op               0 B/op          0 allocs/op

[lemire]

@amikai The performance is ok.

Can you just tweak the small things I flagged ? We will be able to merge.

[amikai]

Hi @lemire, I have tweaked the test.

[lemire]

Great. I am running the CI tests, if it passes, we shall merge and release !!!

[amikai]

Hi @lemire, If there any concerns to merge the PR and release?

[lemire]

Merging!