From 952699d14703a14e2d59b0d8a0af5f0faa812a45 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 20:05:37 -0700 Subject: [PATCH 01/11] Clean up one-off unit tests. Test iterators directly rather than collecting to a slice first. --- kv_test.go | 245 ++++++++++++++++++++++++++++------------------------- 1 file changed, 130 insertions(+), 115 deletions(-) diff --git a/kv_test.go b/kv_test.go index b37ac35..0232e31 100644 --- a/kv_test.go +++ b/kv_test.go @@ -672,152 +672,167 @@ func TestClone(t *testing.T) { // Things that failed at one point or another during testing. -func testFail1(t *testing.T, factory func() TestStore) { - t.Run("fail 1", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{5}, 0) - assert.Equal(t, - []entry{}, - collect(store.Range(From([]byte{5, 0}).To([]byte{6})))) - assert.Equal(t, - []entry{{[]byte{5}, 0}}, - collect(store.Range(From([]byte{4}).To([]byte{5, 0})))) - }) +func assertIterEmpty[V any](t *testing.T, actual iter.Seq2[[]byte, V]) bool { + t.Helper() + for key, value := range actual { + return assert.Fail(t, fmt.Sprintf("should be empty, contains {%s:%v}", kv.KeyName(key), value)) + } + return true } -func testFail2(t *testing.T, factory func() TestStore) { - t.Run("fail 2", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{0xB3, 0x9C}, 184) +func assertIterSingleton[V any](t *testing.T, expectedKey []byte, expectedValue V, actual iter.Seq2[[]byte, V]) bool { + t.Helper() + next, stop := iter.Pull2(actual) + defer stop() + key, value, ok := next() + assert.True(t, ok, "should not be empty") + assert.Equal(t, expectedKey, key) + assert.Equal(t, expectedValue, value) + _, _, ok = next() + assert.False(t, ok, "should have exactly one entry") + return true +} + +func TestFail1(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{5}, 0) + assertIterEmpty(t, store.Range(From([]byte{5, 0}).To([]byte{6}))) + assertIterSingleton(t, []byte{5}, 0, store.Range(From([]byte{4}).To([]byte{5, 0}))) + }) + } +} - // forgot to check isTerminal - actual, actualOk := store.Get([]byte{0xB3}) - assert.False(t, actualOk) - assert.Equal(t, byte(0), actual) +func TestFail2(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{0xB3, 0x9C}, 184) - actual, actualOk = store.Get([]byte{0xB3, 0x9C}) - assert.True(t, actualOk) - assert.Equal(t, byte(184), actual) - }) + // forgot to check isTerminal + actual, actualOk := store.Get([]byte{0xB3}) + assert.False(t, actualOk) + assert.Equal(t, zero, actual) + + actual, actualOk = store.Get([]byte{0xB3, 0x9C}) + assert.True(t, actualOk) + assert.Equal(t, byte(184), actual) + }) + } } -func testFail3(t *testing.T, factory func() TestStore) { - t.Run("fail 3", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{0xB3, 0x9C}, 184) +func TestFail3(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{0xB3, 0x9C}, 184) - actual, actualOk := store.Delete([]byte{0xB3}) - assert.False(t, actualOk) - assert.Equal(t, byte(0), actual) + actual, actualOk := store.Delete([]byte{0xB3}) + assert.False(t, actualOk) + assert.Equal(t, zero, actual) - // Make sure the subtree wasn't deleted. - actual, actualOk = store.Get([]byte{0xB3, 0x9C}) - assert.True(t, actualOk) - assert.Equal(t, byte(184), actual) - }) + // Make sure the subtree wasn't deleted. + actual, actualOk = store.Get([]byte{0xB3, 0x9C}) + assert.True(t, actualOk) + assert.Equal(t, byte(184), actual) + }) + } } -func testFail4(t *testing.T, factory func() TestStore) { - t.Run("fail 4", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{0x50, 0xEF}, 45) - assert.Equal(t, - []entry{}, - collect(store.Range(From([]byte{0x50}).DownTo([]byte{0x15})))) - }) +func TestFail4(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{0x50, 0xEF}, 45) + assertIterEmpty(t, store.Range(From([]byte{0x50}).DownTo([]byte{0x15}))) + }) + } } -func testFail5(t *testing.T, factory func() TestStore) { - t.Run("fail 5", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{0x50, 0xEF}, 45) - assert.Equal(t, - []entry{{[]byte{0x50, 0xEF}, 45}}, - collect(store.Range(From([]byte{0xFD}).DownTo([]byte{0x3D})))) - }) +func TestFail5(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{0x50, 0xEF}, 45) + assertIterSingleton(t, []byte{0x50, 0xEF}, 45, store.Range(From([]byte{0xFD}).DownTo([]byte{0x3D}))) + }) + } } -func testFail6(t *testing.T, factory func() TestStore) { - t.Run("fail 6", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{0x50, 0xEF}, 45) - assert.Equal(t, - []entry{{[]byte{0x50, 0xEF}, 45}}, - collect(store.Range(From([]byte{0x51}).DownTo([]byte{0x50})))) - }) +func TestFail6(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{0x50, 0xEF}, 45) + assertIterSingleton(t, []byte{0x50, 0xEF}, 45, store.Range(From([]byte{0x51}).DownTo([]byte{0x50}))) + }) + } } -func testFail7(t *testing.T, factory func() TestStore) { +func TestFail7(t *testing.T) { // Failure is due to continuing iteration past false yield(). // Failure requires the second Set. - t.Run("fail 7", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{3}, 0) - store.Set([]byte{4}, 0) - assert.Equal(t, - []entry{}, - collect(store.Range(From([]byte{1}).To([]byte{2})))) - }) + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{3}, 0) + store.Set([]byte{4}, 0) + assertIterEmpty(t, store.Range(From([]byte{2}).DownTo([]byte{1}))) + }) + } } -func testFail8(t *testing.T, factory func() TestStore) { - t.Run("fail 8", func(t *testing.T) { - t.Parallel() - store := factory() - store.Set([]byte{}, 1) - store.Set([]byte{0}, 3) - store.Set([]byte{0x23}, 4) - store.Set([]byte{0x23, 0}, 5) - store.Set([]byte{0x23, 0xA5}, 6) - assert.Equal(t, - []entry{{[]byte{0x23, 0}, 5}}, - collect(store.Range(From([]byte{0x23, 0}).To([]byte{0x23, 0, 0})))) - }) +func TestFail8(t *testing.T) { + t.Parallel() + for _, def := range implDefs { + t.Run(def.name, func(t *testing.T) { + t.Parallel() + store := def.factory() + store.Set([]byte{}, 1) + store.Set([]byte{0}, 3) + store.Set([]byte{0x23}, 4) + store.Set([]byte{0x23, 0}, 5) + store.Set([]byte{0x23, 0xA5}, 6) + assertIterSingleton(t, []byte{0x23, 0}, 5, store.Range(From([]byte{0x23, 0}).To([]byte{0x23, 0, 0}))) + }) + } } -func testFail9(t *testing.T, factory func() TestStore) { +func TestFail9(t *testing.T) { // Test that removing the last value on a path removes the path. // Definite hack to detect this one, // but there's no good way to test this using the public API. // The alternative would be to have implementation-specific tests, // which is probably a better approach, but this works for now. - t.Run("fail 9", func(t *testing.T) { - t.Parallel() - store := factory() - sStore, ok := store.(fmt.Stringer) - if !ok { - t.Skipf("%T does not implement Stringer", store) - } - expected := sStore.String() - key := []byte{0x23} - store.Set(key, 6) - store.Delete(key) - assert.Equal(t, expected, sStore.String()) - }) -} - -func TestPastFailures(t *testing.T) { t.Parallel() for _, def := range implDefs { - factory := def.factory t.Run(def.name, func(t *testing.T) { t.Parallel() - testFail1(t, factory) - testFail2(t, factory) - testFail3(t, factory) - testFail4(t, factory) - testFail5(t, factory) - testFail6(t, factory) - testFail7(t, factory) - testFail8(t, factory) - testFail9(t, factory) + store := def.factory() + sStore, ok := store.(fmt.Stringer) + if !ok { + t.Skipf("%T does not implement Stringer", store) + } + expected := sStore.String() + key := []byte{0x23} + store.Set(key, 6) + store.Delete(key) + assert.Equal(t, expected, sStore.String()) }) } } From f70a9041d2f64b5a1ac9eebdf37329adae2af6c9 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 20:09:59 -0700 Subject: [PATCH 02/11] Move traverser-specific test functions to traversers_test.go. --- kv_test.go | 10 ---------- traversers_test.go | 10 ++++++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/kv_test.go b/kv_test.go index 0232e31..6044da7 100644 --- a/kv_test.go +++ b/kv_test.go @@ -236,16 +236,6 @@ func asCloneable(factory func() kv.Store[byte]) func() TestStore { } } -func emptySeqInt(_ func(int) bool) {} - -func emptyAdjInt(_ int) iter.Seq[int] { - return emptySeqInt -} - -func emptyPathAdjInt(_ []int) iter.Seq[int] { - return emptySeqInt -} - func cmpEntryForward(a, b entry) int { return bytes.Compare(a.key, b.key) } diff --git a/traversers_test.go b/traversers_test.go index 0aa1eee..8fc7c76 100644 --- a/traversers_test.go +++ b/traversers_test.go @@ -9,6 +9,16 @@ import ( "github.com/stretchr/testify/assert" ) +func emptySeqInt(_ func(int) bool) {} + +func emptyAdjInt(_ int) iter.Seq[int] { + return emptySeqInt +} + +func emptyPathAdjInt(_ []int) iter.Seq[int] { + return emptySeqInt +} + // adjInt returns a simple adjFunction[int] for testing traversals. // If k <= limit, children(k) == [4*k+1, 4*k+2, 4*k+3]. // If k > limit, children(k) == []. From f21e4c5ca00de469a29ed6bd7171557aa37def09 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 20:17:32 -0700 Subject: [PATCH 03/11] Inline some helper functions that are only used once. --- kv_test.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/kv_test.go b/kv_test.go index 6044da7..4300220 100644 --- a/kv_test.go +++ b/kv_test.go @@ -236,14 +236,6 @@ func asCloneable(factory func() kv.Store[byte]) func() TestStore { } } -func cmpEntryForward(a, b entry) int { - return bytes.Compare(a.key, b.key) -} - -func cmpEntryReverse(a, b entry) int { - return bytes.Compare(b.key, a.key) -} - func collect(itr iter.Seq2[[]byte, byte]) []entry { entries := []entry{} for k, v := range itr { @@ -410,9 +402,9 @@ func assertSame(t *testing.T, entries map[string]byte, store TestStore) { assert.Equal(t, expected, actual) sliceEntries = append(sliceEntries, entry{[]byte(k), expected}) } - slices.SortFunc(sliceEntries, cmpEntryForward) + slices.SortFunc(sliceEntries, func(a, b entry) int { return bytes.Compare(a.key, b.key) }) assert.Equal(t, sliceEntries, collect(store.Range(forwardAll))) - slices.SortFunc(sliceEntries, cmpEntryReverse) + slices.SortFunc(sliceEntries, func(a, b entry) int { return bytes.Compare(b.key, a.key) }) assert.Equal(t, sliceEntries, collect(store.Range(reverseAll))) } From bc1185131dc02da70f5c3f05b92411f55d8bb1d0 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 20:19:52 -0700 Subject: [PATCH 04/11] Move randomization functions to bench_test.go. --- bench_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ kv_test.go | 42 ------------------------------------------ 2 files changed, 40 insertions(+), 42 deletions(-) diff --git a/bench_test.go b/bench_test.go index fc799cd..2c4b25b 100644 --- a/bench_test.go +++ b/bench_test.go @@ -6,6 +6,7 @@ import ( "encoding/binary" "fmt" "maps" + "math" rand "math/rand/v2" "os" "reflect" @@ -48,6 +49,45 @@ var ( benchStoreConfigs = createBenchStoreConfigs() ) +func randomBytes(n int, random *rand.Rand) []byte { + if n == 0 { + return []byte{} + } + k := (n-1)/8 + 1 + b := make([]byte, k*8) + for i := range k { + binary.BigEndian.PutUint64(b[i*8:], random.Uint64()) + } + return b[:n] +} + +func randomByte(random *rand.Rand) byte { + return byte(random.UintN(256)) +} + +// Returns a random key of with length chosen from a roughly normal distribution +// with the given mean. Lengths will range from 0 to 2*mean. +func randomKey(meanLen int, random *rand.Rand) []byte { + const bound = 4.0 // chosen experimentally + val := random.NormFloat64() + for val < -bound || val > +bound { + val = random.NormFloat64() + } + // val is in [-bound, +bound], translate that to [0, 2*mean] + val = (val + bound) * float64(meanLen) / bound + return randomBytes(int(math.Round(val)), random) +} + +func randomFixedLengthKey(keyLen int, random *rand.Rand) []byte { + return randomBytes(keyLen, random) +} + +func shuffle[S ~[]E, E any](slice S, random *rand.Rand) { + random.Shuffle(len(slice), func(i, j int) { + slice[i], slice[j] = slice[j], slice[i] + }) +} + func BenchmarkTraverser(b *testing.B) { benchTraverser(b, "kind=pre-order", kv.TestingPreOrder) benchTraverser(b, "kind=post-order", kv.TestingPostOrder) diff --git a/kv_test.go b/kv_test.go index 4300220..850525e 100644 --- a/kv_test.go +++ b/kv_test.go @@ -2,12 +2,9 @@ package kv_test import ( "bytes" - "encoding/binary" "fmt" "iter" - "math" "math/bits" - rand "math/rand/v2" "reflect" "slices" "strings" @@ -244,45 +241,6 @@ func collect(itr iter.Seq2[[]byte, byte]) []entry { return entries } -func randomBytes(n int, random *rand.Rand) []byte { - if n == 0 { - return []byte{} - } - k := (n-1)/8 + 1 - b := make([]byte, k*8) - for i := range k { - binary.BigEndian.PutUint64(b[i*8:], random.Uint64()) - } - return b[:n] -} - -func randomByte(random *rand.Rand) byte { - return byte(random.UintN(256)) -} - -// Returns a random key of with length chosen from a roughly normal distribution -// with the given mean. Lengths will range from 0 to 2*mean. -func randomKey(meanLen int, random *rand.Rand) []byte { - const bound = 4.0 // chosen experimentally - val := random.NormFloat64() - for val < -bound || val > +bound { - val = random.NormFloat64() - } - // val is in [-bound, +bound], translate that to [0, 2*mean] - val = (val + bound) * float64(meanLen) / bound - return randomBytes(int(math.Round(val)), random) -} - -func randomFixedLengthKey(keyLen int, random *rand.Rand) []byte { - return randomBytes(keyLen, random) -} - -func shuffle[S ~[]E, E any](slice S, random *rand.Rand) { - random.Shuffle(len(slice), func(i, j int) { - slice[i], slice[j] = slice[j], slice[i] - }) -} - // storeConfigs for all possible subsequences of presentKeys. func createTestStoreConfigs() []*storeConfig { result := []*storeConfig{} From 5d155acc2e5477802d8559fbdf86ab9aaee9c7e6 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 20:27:58 -0700 Subject: [PATCH 05/11] Add helper function for testing an early yield(). --- kv_test.go | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/kv_test.go b/kv_test.go index 850525e..d210542 100644 --- a/kv_test.go +++ b/kv_test.go @@ -459,6 +459,7 @@ func TestStoreString(t *testing.T) { } func checkFprint[V any](t *testing.T, expected string, seq iter.Seq2[[]byte, V]) { + t.Helper() var s strings.Builder n, err := kv.Fprint(&s, seq) require.NoError(t, err) @@ -508,7 +509,24 @@ func TestFprint(t *testing.T) { } } -//nolint:gocognit +// This tests that an early yield does not fail, +// and ensures those code paths get test coverage. +func assertEarlyYield(t *testing.T, size int, itr iter.Seq2[[]byte, byte]) { + t.Helper() + expectedCount := size + if expectedCount > 4 { + expectedCount = 4 + } + count := 0 + for range itr { + if count > 3 { + break + } + count++ + } + assert.Equal(t, expectedCount, count) +} + func TestStores(t *testing.T) { t.Parallel() for _, test := range createTestStores(testStoreConfigs) { @@ -549,21 +567,8 @@ func TestStores(t *testing.T) { assert.Equal(t, collect(ref.Range(&bounds)), collect(store.Range(&bounds)), "%s", &bounds) } - // need an early yield for test coverage - count := 0 - for range store.Range(forwardAll) { - if count > 3 { - break - } - count++ - } - count = 0 - for range store.Range(reverseAll) { - if count > 3 { - break - } - count++ - } + assertEarlyYield(t, test.config.size, store.Range(forwardAll)) + assertEarlyYield(t, test.config.size, store.Range(reverseAll)) }) }) } From 4db69e6b6e6b65f9e5f4281b20f74ba07a3ffe20 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 21:08:39 -0700 Subject: [PATCH 06/11] Compare iterators to avoid building slices. --- fuzz_test.go | 16 +++++------ kv_test.go | 81 +++++++++++++++++++++++++++++++++++----------------- 2 files changed, 63 insertions(+), 34 deletions(-) diff --git a/fuzz_test.go b/fuzz_test.go index 4de0fb0..947f9d7 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -69,13 +69,13 @@ func TestBaseline(t *testing.T) { t.Parallel() fuzzStores := createTestStores(fuzzStoreConfigs) ref := createReferenceStore(fuzzStoreConfigs[0]) - refForward := collect(ref.Range(forwardAll)) - refReverse := collect(ref.Range(reverseAll)) + refForward := ref.Range(forwardAll) + refReverse := ref.Range(reverseAll) for _, fuzz := range fuzzStores { t.Run(fuzz.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, refForward, collect(fuzz.store.Range(forwardAll)), "forward") - assert.Equal(t, refReverse, collect(fuzz.store.Range(reverseAll)), "reverse") + assertItersEqual(t, refForward, fuzz.store.Range(forwardAll), "forward") + assertItersEqual(t, refReverse, fuzz.store.Range(reverseAll), "reverse") }) } } @@ -142,11 +142,11 @@ func FuzzRange(f *testing.F) { } forward := From(begin).To(end) reverse := From(end).DownTo(begin) - refForward := collect(ref.Range(forward)) - refReverse := collect(ref.Range(reverse)) + refForward := ref.Range(forward) + refReverse := ref.Range(reverse) for _, fuzz := range fuzzStores { - assert.Equal(t, refForward, collect(fuzz.store.Range(forward)), "%s: %s", fuzz.def.name, forward) - assert.Equal(t, refReverse, collect(fuzz.store.Range(reverse)), "%s: %s", fuzz.def.name, reverse) + assertItersEqual(t, refForward, fuzz.store.Range(forward), "%s: %s", fuzz.def.name, forward) + assertItersEqual(t, refReverse, fuzz.store.Range(reverse), "%s: %s", fuzz.def.name, reverse) } }) } diff --git a/kv_test.go b/kv_test.go index d210542..f5502db 100644 --- a/kv_test.go +++ b/kv_test.go @@ -56,12 +56,6 @@ type ( def *implDef config *storeConfig } - - // Used to test Range result sets. - entry struct { - key []byte - value byte - } ) const ( @@ -233,14 +227,6 @@ func asCloneable(factory func() kv.Store[byte]) func() TestStore { } } -func collect(itr iter.Seq2[[]byte, byte]) []entry { - entries := []entry{} - for k, v := range itr { - entries = append(entries, entry{k, v}) - } - return entries -} - // storeConfigs for all possible subsequences of presentKeys. func createTestStoreConfigs() []*storeConfig { result := []*storeConfig{} @@ -350,20 +336,35 @@ func assertAbsent(t *testing.T, key []byte, store TestStore) { } } +// Temporary. +func entryIter(entries map[string]byte, keys iter.Seq[[]byte]) iter.Seq2[[]byte, byte] { + return func(yield func([]byte, byte) bool) { + for k := range keys { + v, ok := entries[string(k)] + if !ok { + panic(fmt.Sprintf("key %s not found", kv.KeyName(k))) + } + if !yield(k, v) { + return + } + } + } +} + // Test that store contains only the key/value pairs in entries, // and that Range(forward/reverse) returns them in the correct order. func assertSame(t *testing.T, entries map[string]byte, store TestStore) { - sliceEntries := []entry{} - for k, expected := range entries { + keys := [][]byte{} + for k, v := range entries { actual, ok := store.Get([]byte(k)) assert.True(t, ok) - assert.Equal(t, expected, actual) - sliceEntries = append(sliceEntries, entry{[]byte(k), expected}) + assert.Equal(t, v, actual) + keys = append(keys, []byte(k)) } - slices.SortFunc(sliceEntries, func(a, b entry) int { return bytes.Compare(a.key, b.key) }) - assert.Equal(t, sliceEntries, collect(store.Range(forwardAll))) - slices.SortFunc(sliceEntries, func(a, b entry) int { return bytes.Compare(b.key, a.key) }) - assert.Equal(t, sliceEntries, collect(store.Range(reverseAll))) + slices.SortFunc(keys, bytes.Compare) + assertItersEqual(t, entryIter(entries, slices.Values(keys)), store.Range(forwardAll)) + slices.Reverse(keys) + assertItersEqual(t, entryIter(entries, slices.Values(keys)), store.Range(reverseAll)) } func TestNilArgPanics(t *testing.T) { @@ -509,6 +510,36 @@ func TestFprint(t *testing.T) { } } +// Functions to test iterators. + +func msgFrom(msgAndArgs ...any) string { + if len(msgAndArgs) == 0 { + return "" + } + msg, ok := msgAndArgs[0].(string) + if !ok { + return fmt.Sprintf("%+v of type %T must be a format string", msgAndArgs[0], msgAndArgs[0]) + } + return fmt.Sprintf(msg, msgAndArgs[1:]...) +} + +func assertItersEqual[K, V any](t *testing.T, expected, actual iter.Seq2[K, V], msgAndArgs ...any) { + t.Helper() + msg := msgFrom(msgAndArgs...) + i := 0 + next, stop := iter.Pull2(actual) + defer stop() + for expectedKey, expectedValue := range expected { + actualKey, actualValue, ok := next() + require.True(t, ok, "%s: too short, len == %d", msg, i) + assert.Equal(t, expectedKey, actualKey, "%s: keys at index %d differ", msg, i) + assert.Equal(t, expectedValue, actualValue, "%s: values at index %d differ", msg, i) + i++ + } + _, _, ok := next() + assert.False(t, ok, "%s: too long, len > %d", msg, i) +} + // This tests that an early yield does not fail, // and ensures those code paths get test coverage. func assertEarlyYield(t *testing.T, size int, itr iter.Seq2[[]byte, byte]) { @@ -560,12 +591,10 @@ func TestStores(t *testing.T) { t.Run("op=range", func(t *testing.T) { ref := createReferenceStore(test.config) for _, bounds := range test.config.forward { - assert.Equal(t, collect(ref.Range(&bounds)), collect(store.Range(&bounds)), - "%s", &bounds) + assertItersEqual(t, ref.Range(&bounds), store.Range(&bounds), "%s", &bounds) } for _, bounds := range test.config.reverse { - assert.Equal(t, collect(ref.Range(&bounds)), collect(store.Range(&bounds)), - "%s", &bounds) + assertItersEqual(t, ref.Range(&bounds), store.Range(&bounds), "%s", &bounds) } assertEarlyYield(t, test.config.size, store.Range(forwardAll)) assertEarlyYield(t, test.config.size, store.Range(reverseAll)) From 9c332c7c3b44791ad04a6eb3ac30501d97d752e2 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 21:42:24 -0700 Subject: [PATCH 07/11] Structure bench and test names as config first, impl second. This makes benchmarks uncomparable when using benchstat, but new benchmarks need to be run regardless since Go's implementation of map changed significantly. --- bench_test.go | 6 +++--- kv_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/bench_test.go b/bench_test.go index 2c4b25b..65353b7 100644 --- a/bench_test.go +++ b/bench_test.go @@ -455,7 +455,7 @@ func TestBenchStoreConfigRepeatability(t *testing.T) { // This helps to understand how factory() can impact other benchmarks which use it. func BenchmarkFactory(b *testing.B) { for _, def := range implDefs { - b.Run("impl="+def.name, func(b *testing.B) { + b.Run(def.name, func(b *testing.B) { for b.Loop() { _ = def.factory() } @@ -501,7 +501,7 @@ func BenchmarkSparse(b *testing.B) { } shuffle(keys, random) for _, def := range implDefs { - b.Run("impl="+def.name, func(b *testing.B) { + b.Run(def.name, func(b *testing.B) { for b.Loop() { store := def.factory() for _, k := range keys { @@ -543,7 +543,7 @@ func BenchmarkDense(b *testing.B) { {"/keyLen=2", keySets[1]}, {"/keyLen=3", keySets[2]}, } { - b.Run("impl="+def.name+tt.name, func(b *testing.B) { + b.Run(def.name+tt.name, func(b *testing.B) { for b.Loop() { store := def.factory() for _, k := range tt.keys { diff --git a/kv_test.go b/kv_test.go index f5502db..d42a315 100644 --- a/kv_test.go +++ b/kv_test.go @@ -67,9 +67,9 @@ const ( var ( implDefs = []*implDef{ - {"reference", newReference}, - {"pointer-trie", asCloneable(kv.NewPointerTrie[byte])}, - {"array-trie", asCloneable(kv.NewArrayTrie[byte])}, + {"impl=reference", newReference}, + {"impl=pointer-trie", asCloneable(kv.NewPointerTrie[byte])}, + {"impl=array-trie", asCloneable(kv.NewArrayTrie[byte])}, } From = kv.From @@ -288,13 +288,13 @@ func createReferenceStore(config *storeConfig) TestStore { func createTestStores(storeConfigs []*storeConfig) []*testStore { result := []*testStore{} - for _, def := range implDefs { - for _, config := range storeConfigs { + for _, config := range storeConfigs { + for _, def := range implDefs { store := def.factory() for k, v := range config.entries { store.Set([]byte(k), v) } - name := fmt.Sprintf("impl=%s/%s", def.name, config.name) + name := config.name + "/" + def.name result = append(result, &testStore{name, store, def, config}) } } From 48253b64ae27a9765b325a50c9eaef75a424eba9 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 22:38:20 -0700 Subject: [PATCH 08/11] Have reference cache sorted keys. This will speed up range queries when more than one is used. --- kv_test.go | 4 ++ reference_test.go | 157 +++++++++++++++++++++++++++++++++------------- 2 files changed, 116 insertions(+), 45 deletions(-) diff --git a/kv_test.go b/kv_test.go index d42a315..028d7e5 100644 --- a/kv_test.go +++ b/kv_test.go @@ -806,6 +806,10 @@ func TestFail9(t *testing.T) { key := []byte{0x23} store.Set(key, 6) store.Delete(key) + // make sure reference.dirty is false + for range store.Range(forwardAll) { + break + } assert.Equal(t, expected, sStore.String()) }) } diff --git a/reference_test.go b/reference_test.go index 5066c4a..ad327bb 100644 --- a/reference_test.go +++ b/reference_test.go @@ -1,7 +1,7 @@ package kv_test import ( - "cmp" + "bytes" "fmt" "iter" "maps" @@ -11,88 +11,155 @@ import ( "github.com/phiryll/kv" ) +// reference serves as an expected value to compare against while testing, +// and a source of entries from which to create a new Store. +// This implementation is meant to be as trivially correct as possible. +type reference struct { + entries map[string]byte + ascKeys [][]byte + dirty bool +} + func newReference() TestStore { - return reference{} + return &reference{ + entries: map[string]byte{}, + } } -// reference implements the TestStore interface, but it is not a trie. -// This serves as an expected value to compare against a Store[byte] implementation while testing. -type reference map[string]byte +func (r *reference) Clone() TestStore { + return &reference{ + maps.Clone(r.entries), + slices.Clone(r.ascKeys), + r.dirty, + } +} -func (r reference) Clone() TestStore { - return maps.Clone(r) +func (r *reference) refresh() { + if !r.dirty { + return + } + var keys [][]byte + for key := range r.entries { + keys = append(keys, []byte(key)) + } + slices.SortFunc(keys, bytes.Compare) + r.ascKeys = keys + r.dirty = false } -func (r reference) Set(key []byte, value byte) (byte, bool) { +func (r *reference) Get(key []byte) (byte, bool) { if key == nil { panic("key must be non-nil") } - index := string(key) - prev, ok := r[index] - r[index] = value - if ok { - return prev, true - } - return 0, false + value, ok := r.entries[string(key)] + return value, ok } -func (r reference) Get(key []byte) (byte, bool) { +func (r *reference) Set(key []byte, value byte) (byte, bool) { if key == nil { panic("key must be non-nil") } - value, ok := r[string(key)] - return value, ok + prev, ok := r.entries[string(key)] + r.entries[string(key)] = value + r.ascKeys = nil + r.dirty = true + return prev, ok } -func (r reference) Delete(key []byte) (byte, bool) { +func (r *reference) Delete(key []byte) (byte, bool) { if key == nil { panic("key must be non-nil") } - index := string(key) - value, ok := r[index] - delete(r, index) + value, ok := r.entries[string(key)] + if ok { + delete(r.entries, string(key)) + r.ascKeys = nil + r.dirty = true + } return value, ok } -// Does not work with NaN. -func negCompare[T cmp.Ordered](x, y T) int { - if x < y { - return +1 +func (r *reference) Range(bounds *Bounds) iter.Seq2[[]byte, byte] { + bounds = bounds.Clone() + if bounds.IsReverse { + return r.Desc(bounds.End, bounds.Begin) } - if x > y { - return -1 + return r.Asc(bounds.Begin, bounds.End) +} + +// Future methods on Store, replacing Range. +// Desc will behave differently, with low inclusive and high exclusive. + +func (r *reference) All() iter.Seq2[[]byte, byte] { + return func(yield func([]byte, byte) bool) { + for k, v := range r.entries { + if !yield([]byte(k), v) { + return + } + } } - return 0 } -func (r reference) Range(bounds *Bounds) iter.Seq2[[]byte, byte] { - bounds = bounds.Clone() +func (r *reference) between(low, high []byte) [][]byte { + lowIndex, highIndex := 0, len(r.ascKeys) + if low != nil { + lowIndex, _ = slices.BinarySearchFunc(r.ascKeys, low, bytes.Compare) + } + if high != nil { + highIndex, _ = slices.BinarySearchFunc(r.ascKeys, high, bytes.Compare) + } + return r.ascKeys[lowIndex:highIndex] +} + +func (r *reference) Asc(low, high []byte) iter.Seq2[[]byte, byte] { + if low != nil && high != nil && bytes.Compare(low, high) >= 0 { + panic("low >= high") + } return func(yield func([]byte, byte) bool) { - var keys []string - for k := range r { - if bounds.CompareKey([]byte(k)) == 0 { - keys = append(keys, k) + r.refresh() + for _, key := range r.between(low, high) { + if !yield(key, r.entries[string(key)]) { + return } } - if bounds.IsReverse { - slices.SortFunc(keys, negCompare) - } else { - slices.Sort(keys) + } +} + +func (r *reference) Desc(low, high []byte) iter.Seq2[[]byte, byte] { + if low != nil && high != nil && bytes.Compare(low, high) >= 0 { + panic("low >= high") + } + return func(yield func([]byte, byte) bool) { + r.refresh() + a, b := low, high + if a != nil { + a = nextKey(a) + } + if b != nil { + b = nextKey(b) } - for _, k := range keys { - if !yield([]byte(k), r[k]) { + for _, key := range slices.Backward(r.between(a, b)) { + if !yield(key, r.entries[string(key)]) { return } } } } -func (r reference) String() string { +func (r *reference) String() string { var s strings.Builder - s.WriteString("{") - for _, k := range slices.Sorted(maps.Keys(r)) { - fmt.Fprintf(&s, "%s:%v, ", kv.KeyName([]byte(k)), r[k]) + s.WriteString("{\n") + fmt.Fprintf(&s, " dirty: %v\n", r.dirty) + s.WriteString(" entries: {") + for key, value := range r.entries { + fmt.Fprintf(&s, "%s:%02X, ", kv.KeyName([]byte(key)), value) + } + s.WriteString(" }\n") + s.WriteString(" ascKeys: {") + for _, key := range r.ascKeys { + fmt.Fprintf(&s, "%s, ", kv.KeyName(key)) } + s.WriteString(" }\n") s.WriteString("}") return s.String() } From 24e5a88e246c319fd8f2fa29dee08062251ac706 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 22:55:11 -0700 Subject: [PATCH 09/11] No need to clone the store in BenchmarkGet. --- bench_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/bench_test.go b/bench_test.go index 65353b7..0b0c37f 100644 --- a/bench_test.go +++ b/bench_test.go @@ -598,7 +598,6 @@ func BenchmarkSet(b *testing.B) { //nolint:gocognit func BenchmarkGet(b *testing.B) { for _, bench := range createTestStores(benchStoreConfigs) { - original := bench.store b.Run(bench.name, func(b *testing.B) { for keyLen := 8; keyLen < len(bench.config.present); keyLen += 4 { b.Run(fmt.Sprintf("keyLen=%d/existing=true", keyLen), func(b *testing.B) { @@ -606,10 +605,9 @@ func BenchmarkGet(b *testing.B) { if len(present) == 0 { b.Skipf("no present keys of length %d", keyLen) } - store := original.Clone() i := 0 for b.Loop() { - store.Get(present[i%len(present)]) + bench.store.Get(present[i%len(present)]) i++ } }) @@ -618,10 +616,9 @@ func BenchmarkGet(b *testing.B) { if len(absent) == 0 { b.Skipf("no absent keys of length %d", keyLen) } - store := original.Clone() i := 0 for b.Loop() { - store.Get(absent[i%len(absent)]) + bench.store.Get(absent[i%len(absent)]) i++ } }) From 9587b83ce75953d0bef3f547521edf83c5473127 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 23:06:22 -0700 Subject: [PATCH 10/11] Remove some benchmarks for ChildBounds. --- bench_test.go | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/bench_test.go b/bench_test.go index 0b0c37f..1ef59c9 100644 --- a/bench_test.go +++ b/bench_test.go @@ -156,6 +156,8 @@ func BenchmarkChildBounds(b *testing.B) { bounds *Bounds keys keySet }{ + // No need to benchmark reverse bounds, the code is the same. + // no common prefix { From(nil).To(empty), @@ -167,14 +169,11 @@ func BenchmarkChildBounds(b *testing.B) { }, { From(nil).To(high), - keySet{ - empty, nextKey(empty), before, high[:1], high[:2], high[:3], - prevKey(high), high, nextKey(high), after, - }, + keySet{empty, high[:1], high[:2], high[:3], prevKey(high), high, nextKey(high), after}, }, { From(nil).To(nil), - keySet{empty, nextKey(empty), within}, + keySet{empty, nextKey(empty), after}, }, { From(empty).To(nextKey(empty)), @@ -182,36 +181,33 @@ func BenchmarkChildBounds(b *testing.B) { }, { From(empty).To(high), - keySet{ - empty, nextKey(empty), before, high[:1], high[:2], high[:3], - prevKey(high), high, nextKey(high), after, - }, + keySet{empty, nextKey(empty), high[:1], high[:2], high[:3], prevKey(high), high, nextKey(high), after}, }, { From(empty).To(nil), - keySet{empty, nextKey(empty), within}, + keySet{empty, nextKey(empty), after}, }, { From(nextKey(empty)).To(high), keySet{ - empty, nextKey(empty), nextKey(nextKey(empty)), before, high[:1], high[:2], high[:3], + empty, nextKey(empty), nextKey(nextKey(empty)), high[:1], high[:2], high[:3], prevKey(high), high, nextKey(high), after, }, }, { From(nextKey(empty)).To(nil), - keySet{empty, nextKey(empty), nextKey(nextKey(empty)), within}, + keySet{empty, nextKey(empty), nextKey(nextKey(empty)), after}, }, { From(low).To(high), keySet{ - empty, nextKey(empty), before, low[:1], low[:2], low[:3], prevKey(low), low, nextKey(low), - within, high[:1], high[:2], high[:3], prevKey(high), high, nextKey(high), after, + empty, nextKey(empty), low[:1], low[:2], low[:3], prevKey(low), low, nextKey(low), + high[:1], high[:2], high[:3], prevKey(high), high, nextKey(high), after, }, }, { From(low).To(nil), - keySet{empty, nextKey(empty), before, low[:1], low[:2], low[:3], prevKey(low), low, nextKey(low), after}, + keySet{empty, nextKey(empty), low[:1], low[:2], low[:3], prevKey(low), low, nextKey(low), after}, }, // 2 byte common prefix @@ -233,20 +229,11 @@ func BenchmarkChildBounds(b *testing.B) { }, } { b.Run(fmt.Sprintf("bounds=%s", tt.bounds), func(b *testing.B) { - forward := tt.bounds - reverse := From(tt.bounds.End).DownTo(tt.bounds.Begin) for _, k := range tt.keys { b.Run("key="+kv.KeyName(k), func(b *testing.B) { - b.Run("dir=forward", func(b *testing.B) { - for b.Loop() { - kv.TestingChildBounds(forward, k) - } - }) - b.Run("dir=reverse", func(b *testing.B) { - for b.Loop() { - kv.TestingChildBounds(reverse, k) - } - }) + for b.Loop() { + kv.TestingChildBounds(tt.bounds, k) + } }) } }) From 89f24e106047238c370588bd7c25e0a859e575d7 Mon Sep 17 00:00:00 2001 From: Jennifer Conner <2819667+phiryll@users.noreply.github.com> Date: Fri, 4 Jul 2025 23:26:27 -0700 Subject: [PATCH 11/11] Remove some unneeded unit tests. --- bench_test.go | 48 ------------------------------------------------ fuzz_test.go | 15 --------------- kv_test.go | 8 -------- 3 files changed, 71 deletions(-) diff --git a/bench_test.go b/bench_test.go index 1ef59c9..befdd0d 100644 --- a/bench_test.go +++ b/bench_test.go @@ -5,17 +5,14 @@ import ( "bytes" "encoding/binary" "fmt" - "maps" "math" rand "math/rand/v2" "os" - "reflect" "slices" "strings" "testing" "github.com/phiryll/kv" - "github.com/stretchr/testify/assert" ) // No benchmark can have a truly random element, random seeds must be constants! @@ -394,51 +391,6 @@ func createBenchStoreConfigs() []*storeConfig { return append(createBenchRandomStoreConfigs(), createBenchWordStoreConfigs()...) } -func TestBenchStoreConfigs(t *testing.T) { - t.Parallel() - for _, config := range benchStoreConfigs { - t.Run(config.name, func(t *testing.T) { - t.Parallel() - assert.Len(t, config.entries, config.size) - assert.Equal(t, len(config.present), len(config.absent)) - assert.Equal(t, 1, len(config.present[0])+len(config.absent[0])) - assert.Equal(t, 1<<8, len(config.present[1])+len(config.absent[1])) - assert.Equal(t, 1<<16, len(config.present[2])+len(config.absent[2])) - assert.Len(t, config.forward, 1<<16) - assert.Len(t, config.reverse, 1<<16) - - present := maps.Clone(config.entries) - for i := range len(config.present) { - if i > 2 { - assert.Len(t, config.absent[i], 1<<16) - } - for _, k := range config.absent[i] { - assert.Len(t, k, i) - _, ok := present[string(k)] - assert.False(t, ok) - } - for _, k := range config.present[i] { - assert.Len(t, k, i) - _, ok := present[string(k)] - assert.True(t, ok) - delete(present, string(k)) - } - } - assert.Empty(t, present) - }) - } -} - -func TestBenchStoreConfigRepeatability(t *testing.T) { - t.Parallel() - for i, config := range createBenchStoreConfigs() { - t.Run(config.name, func(t *testing.T) { - t.Parallel() - assert.True(t, reflect.DeepEqual(benchStoreConfigs[i], config)) - }) - } -} - // This helps to understand how factory() can impact other benchmarks which use it. func BenchmarkFactory(b *testing.B) { for _, def := range implDefs { diff --git a/fuzz_test.go b/fuzz_test.go index 947f9d7..e1afc8b 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -65,21 +65,6 @@ func createFuzzStoreConfigs(size int) []*storeConfig { return []*storeConfig{&config} } -func TestBaseline(t *testing.T) { - t.Parallel() - fuzzStores := createTestStores(fuzzStoreConfigs) - ref := createReferenceStore(fuzzStoreConfigs[0]) - refForward := ref.Range(forwardAll) - refReverse := ref.Range(reverseAll) - for _, fuzz := range fuzzStores { - t.Run(fuzz.name, func(t *testing.T) { - t.Parallel() - assertItersEqual(t, refForward, fuzz.store.Range(forwardAll), "forward") - assertItersEqual(t, refReverse, fuzz.store.Range(reverseAll), "reverse") - }) - } -} - func FuzzGet(f *testing.F) { fuzzStores := createTestStores(fuzzStoreConfigs) ref := createReferenceStore(fuzzStoreConfigs[0]) diff --git a/kv_test.go b/kv_test.go index 028d7e5..e80f296 100644 --- a/kv_test.go +++ b/kv_test.go @@ -5,7 +5,6 @@ import ( "fmt" "iter" "math/bits" - "reflect" "slices" "strings" "testing" @@ -271,13 +270,6 @@ func createTestStoreConfigs() []*storeConfig { return result } -func TestTestStoreConfigRepeatability(t *testing.T) { - t.Parallel() - for i, config := range createTestStoreConfigs() { - assert.True(t, reflect.DeepEqual(testStoreConfigs[i], config)) - } -} - func createReferenceStore(config *storeConfig) TestStore { store := newReference() for k, v := range config.entries {