From 3c4ab7a0691f94899c8215f2cb0202c6db45a7af Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 16 Mar 2023 13:25:55 +0000 Subject: [PATCH 1/2] labels: add test for Builder.Range Including mutating the Builder being Ranged over. Signed-off-by: Bryan Boreham --- model/labels/labels_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/model/labels/labels_test.go b/model/labels/labels_test.go index 4832be337..588a84b98 100644 --- a/model/labels/labels_test.go +++ b/model/labels/labels_test.go @@ -529,6 +529,11 @@ func TestBuilder(t *testing.T) { base: FromStrings("aaa", "111"), want: FromStrings("aaa", "111"), }, + { + base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"), + set: []Label{{"aaa", "444"}, {"bbb", "555"}, {"ccc", "666"}}, + want: FromStrings("aaa", "444", "bbb", "555", "ccc", "666"), + }, { base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"), del: []string{"bbb"}, @@ -591,7 +596,15 @@ func TestBuilder(t *testing.T) { b.Keep(tcase.keep...) } b.Del(tcase.del...) - require.Equal(t, tcase.want, b.Labels(tcase.base)) + require.Equal(t, tcase.want, b.Labels(EmptyLabels())) + + // Check what happens when we call Range and mutate the builder. + b.Range(func(l Label) { + if l.Name == "aaa" || l.Name == "bbb" { + b.Del(l.Name) + } + }) + require.Equal(t, tcase.want.BytesWithoutLabels(nil, "aaa", "bbb"), b.Labels(tcase.base).Bytes(nil)) }) } } From 3743d87c56a610fbcdd7d49e6f85e2e873d37bc5 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 16 Mar 2023 13:28:13 +0000 Subject: [PATCH 2/2] labels: cope with mutating Builder during Range call Although we had a different slice, the underlying memory was the same so any changes meant we could skip some values. Signed-off-by: Bryan Boreham --- model/labels/labels.go | 7 +++++-- model/labels/labels_string.go | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/model/labels/labels.go b/model/labels/labels.go index 5e06e3b8d..6de001c3c 100644 --- a/model/labels/labels.go +++ b/model/labels/labels.go @@ -545,9 +545,12 @@ func (b *Builder) Get(n string) string { } // Range calls f on each label in the Builder. -// If f calls Set or Del on b then this may affect what callbacks subsequently happen. func (b *Builder) Range(f func(l Label)) { - origAdd, origDel := b.add, b.del + // Stack-based arrays to avoid heap allocation in most cases. + var addStack [1024]Label + var delStack [1024]string + // Take a copy of add and del, so they are unaffected by calls to Set() or Del(). + origAdd, origDel := append(addStack[:0], b.add...), append(delStack[:0], b.del...) b.base.Range(func(l Label) { if !slices.Contains(origDel, l.Name) && !contains(origAdd, l.Name) { f(l) diff --git a/model/labels/labels_string.go b/model/labels/labels_string.go index 6fe14bedc..98db29d25 100644 --- a/model/labels/labels_string.go +++ b/model/labels/labels_string.go @@ -599,9 +599,12 @@ func (b *Builder) Get(n string) string { } // Range calls f on each label in the Builder. -// If f calls Set or Del on b then this may affect what callbacks subsequently happen. func (b *Builder) Range(f func(l Label)) { - origAdd, origDel := b.add, b.del + // Stack-based arrays to avoid heap allocation in most cases. + var addStack [1024]Label + var delStack [1024]string + // Take a copy of add and del, so they are unaffected by calls to Set() or Del(). + origAdd, origDel := append(addStack[:0], b.add...), append(delStack[:0], b.del...) b.base.Range(func(l Label) { if !slices.Contains(origDel, l.Name) && !contains(origAdd, l.Name) { f(l)