Add generic buffer.TypedRingGrowing and shrinkable buffer.Ring #323

nojnhuh · 2025-03-25T03:24:46Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR adds a new type, buffer.Ring which is a buffer.RingGrowing with a type parameter (generics) and the capability to shrink its underlying buffer when all elements have been read. It should be able to replace the scheduler's use of queue.FIFO (and its copy in k8s.io/dynamic-resource-allocation/internal/queue). Behavior of the existing buffer.RingGrowing should be unchanged.

Which issue(s) this PR fixes:

First step of kubernetes/kubernetes#131032

Special notes for your reviewer:

I verified that this new implementation passes the existing unit tests for k8s.io/client-go/tools/cache where buffer.RingGrowing is currently in use without any changes to that package, and for k8s.io/kubernetes/pkg/scheduler/util/assumecache and k8s.io/dynamic-resource-allocation/resourceslice/tracker replacing its use of queue.FIFO: kubernetes/kubernetes@master...nojnhuh:kubernetes:typed-ring-buffer.

Release note:

Add new, generic buffer.TypedRingGrowing type to succeed buffer.RingGrowing which is now deprecated. Add new buffer.Ring type, which is equivalent to buffer.TypedRingGrowing, but shrinks to its original size after its elements have been totally consumed.

buffer/ring_growing.go

nojnhuh · 2025-03-25T03:27:11Z

/cc @pohly

nojnhuh · 2025-04-14T04:03:09Z

AFAICT the apidiff failure is a false-positive, maybe it doesn't fully understand generics? I've at least checked that client-go will still compile without any changes with this PR.

@apelisse @cheftako If you could PTAL that would be great, thanks!

buffer/ring_growing.go

pohly · 2025-05-20T18:13:18Z

/wg device-management
/priority important-soon

We need this to replace some custom FIFO implementation in two different places (scheduler plugin and ResourceSlice tracker). Would be nice to get into 1.34 soon to give us time to adapt k/k.

buffer/ring_growing.go

pohly

Looks good to me with perhaps one small doc tweak.

Please squash into one commit to make it ready for merging.

nojnhuh · 2025-05-20T18:22:14Z

Looks good to me with perhaps one small doc tweak.

Please squash into one commit to make it ready for merging.

Updated and squashed.

pohly

/lgtm

I think all my comments and concerns have been addressed. But it's been a while, so a second top-to-bottom pass from an approver would be useful.

pohly · 2025-05-21T06:00:50Z

/assign @aojea

For a second review and approval.

aojea · 2025-05-27T10:55:47Z

buffer/ring_growing.go

 	}
 	r.readable--
 	element := r.data[r.beg]
-	r.data[r.beg] = nil // Remove reference to the object to help GC
+	var zero T
+	r.data[r.beg] = zero // Remove reference to the object to help GC


this is only true if T is a pointer, no?

Not really. e.g. string, slice, map, any struct that has such fields and/or pointers.

(I'm not the author of the PR but I wrote the original code)

ok, yeah , I mean , if we have an int this will be 0 , right :) ... maybe pedantic, or nitpicking, just for correctness

@ash2k: perhaps you can help with the review then?

@pohly I did have a look earlier today and I have the same questions re. allocations. Overall it looks good.

aojea · 2025-05-27T10:58:51Z

buffer/ring_growing.go

+		if newN == 0 {
+			newN = 1
+		}
+		newData := make([]T, newN)


why before we didn't initialize to 1 when it was zero?

My understanding is that before this PR we assumed that the initial size is some non-zero value. With this PR the RingOptions may not have the InitialSize set. In that case the NewRing constructor will use 0 as the initial size and multiplying by 2 wouldn't grow it :)

I think it'd be better to use some sane defaults for all parameters in the constructor rather than hacking around like this in the middle of the logic.

Same with NormalSize - I don't think it makes sense to shrink to 0 and then grow to 1, 2, 4, 8, etc. Why not use some sane size if nothing was provided? Maybe 32 or something sufficiently large.

I agree that an initial size and normal size of 0 is probably not ideal in most cases. For something as generic as this though, I don't think we can realistically determine "sane" defaults for initial size or normal size for everywhere this could be used.

Would it be better to require the initial size and normal size to be set? That would at least prevent users from assuming the implicit default of 0 for each.

Current default is effectively 1 for the initial size. We all seem to agree this is not a good default for any use case. I think it'd be beneficial to have the default set (for both) to 16 or 32 - even if the buffer is not used that much, it's not wasting a lot of space. If this default does not work for someone, they can set the value they want explicitly (just as you are suggesting). WDYT?

If 0 isn't good enough (which I think is similar to how the Go runtime behaves with plain slices), I'd still prefer requiring those parameters be set by the user over trying to determine defaults which are more acceptable.

I think it'd be nice to keep the zero value of the type a valid, working instance i.e. ideally it shouldn't blow up if was not created via the constructor.

If we agree on the above, then we only need to change that if to use some other value, not 1. But what about normalSize? Isn't there a performance bug now where any buffer would be thrown away once empty and replaced with a zero length slice when default values are used? This is not ideal, right? And if we write to it, it'll have to immediately grow/allocate the slice again. And then if we read - it'll throw away the slice again. So, we have a pathological case where each read of the last element generates garbage for GC. Am I missing something?

If we want to solve the above, we have to treat normalSize==0 as "use a default value" for this parameter too. What is the default here? We have to come up with two values now. 16 for both?

One more thing: does it make sense to allow normalSize to be smaller than initialSize? We don't want to start with e.g. 100 and then shrink to e.g. 40 when it's empty. That just doesn't make sense. Shall we set normalSize=max(normalSize, initialSize)?

those are good questions, the problem to put this in this repo is that there is a strong assumption of stability and correctness, if this is going to be used only in one place why don't you create this only for self consumption in the k/k repo? just putting it here to revendor for k/k adds a lot of maintanance burden. ...

EDIT answering myself, this came here because of the opposite reasons kubernetes/kubernetes#73209 (comment)

If we agree on the above, then we only need to change that if to use some other value, not 1. But what about normalSize? Isn't there a performance bug now where any buffer would be thrown away once empty and replaced with a zero length slice when

what about I think that provide some more obvious default, I put 16 as it seems we are leaning towards that value, no other reason

diff --git a/buffer/ring_growing.go b/buffer/ring_growing.go index 97dcd6e..0f6d31d 100644 --- a/buffer/ring_growing.go +++ b/buffer/ring_growing.go @@ -16,6 +16,9 @@ limitations under the License. package buffer +// defaultRingSize defines the default ring size if not specified +const defaultRingSize = 16 + // RingGrowingOptions sets parameters for [RingGrowing] and // [TypedRingGrowing]. type RingGrowingOptions struct { @@ -79,7 +82,7 @@ func (r *TypedRingGrowing[T]) WriteOne(data T) { // Time to grow newN := r.n * 2 if newN == 0 { - newN = 1 + newN = defaultRingSize } newData := make([]T, newN) to := r.beg + r.readable

One more thing: does it make sense to allow normalSize to be smaller than initialSize? We don't want to start with e.g. 100 and then shrink to e.g. 40 when it's empty.

I'm not fan of offering an API that is able to do that and then change the behavior behind the scenes, despite totally agree does not make sense I expect that if someone sets those parameters is because they want just to do that

Made that change to add a bigger default and also added the benchmarks.

buffer/ring_growing.go

aojea · 2025-05-27T11:03:17Z

buffer/ring_growing.go

+	if r.growing.readable == 0 && r.growing.n > r.normalSize {
+		// The buffer is empty. Reallocate a new buffer so the old one can be
+		// garbage collected.
+		r.growing.data = make([]T, r.normalSize)
+		r.growing.n = r.normalSize
+		r.growing.beg = 0
+	}


is this effective? allocating and deallocating memory vs reusing it?

The way I understood this change is that the idea is to shrink the hugely expanded buffer back to some typical size. E.g. there was a spike of usage (as it happens when e.g. a controller starts and fills up a workqueue but the workers only start when all informers have synced). Waiting for it to get to 0 first allows to eliminate the need for copying the data, which is good.

I.e. it is less effective but the goal is to reduce the amount of ram used so we have to free the "big" buffer and allocate a new one that is "normal".

is this effective? allocating and deallocating memory vs reusing it?

A new, smaller array gets allocated here so the old, larger array can be garbage collected. I don't think it's possible to inform the Go runtime that part of an array backing a slice can be garbage collected, only that the entire backing array can be by removing all references to any part of the backing array.

ash2k · 2025-05-28T09:44:52Z

/lgtm

aojea · 2025-05-29T10:25:24Z

ok, this seems to work as expected, it sacrifices allocations for long term memory usage

const (
	spikeSize   = 100 // Number of items to write during a spike
	normalSize  = 64  // Normal capacity for the Ring type after shrinking
	initialSize = 16  // Initial capacity for buffers
)

func BenchmarkTypedRingGrowing_Spike(b *testing.B) {
	b.ReportAllocs()
	var item int // ensure item is used

	for i := 0; i < b.N; i++ {
		buffer := NewTypedRingGrowing[int](RingGrowingOptions{InitialSize: initialSize})

		for j := 0; j < spikeSize; j++ {
			buffer.WriteOne(j)
		}

		for buffer.Len() > 0 {
			item, _ = buffer.ReadOne()
		}
	}
	_ = item // use item
}

func BenchmarkRing_Spike_And_Shrink(b *testing.B) {
	b.ReportAllocs()
	var item int // ensure item is used

	for i := 0; i < b.N; i++ {
		// Create a new buffer for each benchmark iteration
		buffer := NewRing[int](RingOptions{
			InitialSize: initialSize,
			NormalSize:  normalSize,
		})

		for j := 0; j < spikeSize; j++ {
			buffer.WriteOne(j)
		}

		for buffer.Len() > 0 {
			item, _ = buffer.ReadOne()
		}
	}
	_ = item // use item
}

go test  -run=^$ -bench=. k8s.io/utils/buffer -v -count 1
goos: linux
goarch: amd64
pkg: k8s.io/utils/buffer
cpu: Intel(R) Xeon(R) CPU @ 2.60GHz
BenchmarkTypedRingGrowing_Spike
BenchmarkTypedRingGrowing_Spike-48        953743              1094 ns/op            1920 B/op          4 allocs/op
BenchmarkRing_Spike_And_Shrink
BenchmarkRing_Spike_And_Shrink-48         819721              1325 ns/op            2432 B/op          5 allocs/op
PASS
ok      k8s.io/utils/buffer     2.166s

The apidiff does not seems to cause any issue with the existing code as it is able to infer the type IIRC
It also fixes a bug where a buffer not initialized or initialized as zero will not be able to grow

There is an outstanding discussion about the default size, #323 (comment)

aojea · 2025-05-29T12:46:18Z

+1 if we change the magic 1 by a constant and we use a more larger default as 16 per example https://github.com/kubernetes/utils/pull/323/files#r2113869115

Co-authored-by: Antonio Ojea <[email protected]>

aojea · 2025-05-30T05:06:30Z

buffer/ring_growing_test.go

+			if expected, actual := x, g.Len(); expected != actual {
+				t.Fatalf("expected Len to be %d, got %d", expected, actual)
+			}
+			if expected, actual := 16, g.Cap(); expected != actual {


note to self, this is 16 because is the next power of 2 that can accomodate 10 elements, x is 10

aojea · 2025-05-30T05:08:01Z

buffer/ring_growing_test.go

+				if read != v {
+					t.Fatalf("expected %#v==%#v", read, v)
+				}
+				read++


note to self, we write the sequence of the index for loop, 1 2 3 ... so we check we read exactly those values

aojea · 2025-05-30T05:08:37Z

buffer/ring_growing_test.go

+			if x != read {
+				t.Fatalf("expected to have read %d items: %d", x, read)
+			}
+			if expected, actual := 0, g.Len(); expected != actual {


buffer should be empty

aojea · 2025-05-30T05:09:21Z

buffer/ring_growing_test.go

+			if expected, actual := 0, g.Len(); expected != actual {
+				t.Fatalf("expected Len to be %d, got %d", expected, actual)
+			}
+			if expected, actual := normalSize, g.Cap(); expected != actual {


but the capacity should be equal to the configured normal size

aojea · 2025-05-30T05:19:08Z

/approve

@ash2k for final lgtm

Thanks

ash2k · 2025-05-30T09:18:39Z

/lgtm

Thank you, @nojnhuh!

k8s-ci-robot · 2025-05-30T09:18:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, ash2k, nojnhuh, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aojea]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aojea · 2025-05-30T09:42:45Z

/override

k8s-ci-robot · 2025-05-30T09:42:48Z

@aojea: /override requires failed status contexts to operate on, but none was given

In response to this:

/override

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

aojea · 2025-05-30T09:44:52Z

/override apidiff

The apidiff does not seems to cause any issue with the existing code as it is able to infer the type IIRC

k8s-ci-robot · 2025-05-30T09:44:56Z

@aojea: aojea unauthorized: /override is restricted to Repo administrators.

In response to this:

/override apidiff

The apidiff does not seems to cause any issue with the existing code as it is able to infer the type IIRC

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

aojea · 2025-05-30T09:45:14Z

@dims we need your help :)

aojea · 2025-05-30T09:50:12Z

hmm, this is an incompatible change based on https://go-review.googlesource.com/c/tools/+/618215/3/internal/apidiff/testdata/tests.go#535

@nojnhuh should we leave the old buffer as it was to avoid breaking compatibility?

aojea · 2025-05-30T09:50:54Z

buffer/ring_growing.go

-	data     []interface{}
+//
+// Deprecated: Use TypedRingGrowing[any] instead.
+type RingGrowing = TypedRingGrowing[any]


this seem to break compatibility https://go-review.googlesource.com/c/tools/+/618215/3/internal/apidiff/testdata/tests.go#535

In our case we don't have ~ on the type parameter, so maybe it's not a problem? https://go.dev/play/p/cTywdiHoQp_l this works fine (removed ~).

A similar change was made to gRPC and it seems nothing broke: https://github.com/grpc/grpc-go/pull/7057/files#diff-c5003637b707b222097960cf01b1d09d77126e39ff0073bff748bbc84951e6cfR74.

Disclaimer: I'm not an expert on Go generics by any means.

me neither, just why I'm broadcasting :) I feel that since this is only used in one place and I already tested that there is no need to adapt the code after vendor with this change, we can override , @dims what do you think

@aojea @ash2k as long as you can confirm that just updating the library does not break existing code, we can land this. looks like you both have tried it already. So we should be good to go.

I did replace the vendor in k/k with this branch and binaries compiled just fine

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 25, 2025

k8s-ci-robot requested review from apelisse and cheftako March 25, 2025 03:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 25, 2025

nojnhuh commented Mar 25, 2025

View reviewed changes

buffer/ring_growing.go Outdated Show resolved Hide resolved

buffer/ring_growing.go Show resolved Hide resolved

k8s-ci-robot requested a review from pohly March 25, 2025 03:27

pohly reviewed Apr 29, 2025

View reviewed changes

buffer/ring_growing.go Outdated Show resolved Hide resolved

k8s-ci-robot added wg/device-management Categorizes an issue or PR as relevant to WG Device Management. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 20, 2025

pohly reviewed May 20, 2025

View reviewed changes

buffer/ring_growing.go Outdated Show resolved Hide resolved

pohly reviewed May 20, 2025

View reviewed changes

nojnhuh force-pushed the typed-ring-buffer branch from 133f6ff to bb2bb1b Compare May 20, 2025 18:22

nojnhuh force-pushed the typed-ring-buffer branch from bb2bb1b to 69bc812 Compare May 20, 2025 18:27

pohly reviewed May 20, 2025

View reviewed changes

k8s-ci-robot assigned pohly May 20, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2025

pohly moved this to 👀 In review in Dynamic Resource Allocation May 21, 2025

pohly added this to Dynamic Resource Allocation May 21, 2025

k8s-ci-robot assigned aojea May 21, 2025

aojea reviewed May 27, 2025

View reviewed changes

buffer/ring_growing.go Outdated Show resolved Hide resolved

aojea reviewed May 27, 2025

View reviewed changes

nojnhuh force-pushed the typed-ring-buffer branch from 69bc812 to ed4fc2c Compare May 27, 2025 20:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2025

k8s-ci-robot assigned ash2k May 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2025

Add generic buffer.TypedRingGrowing and shrinkable buffer.Ring

aace85d

Co-authored-by: Antonio Ojea <[email protected]>

nojnhuh force-pushed the typed-ring-buffer branch from ed4fc2c to aace85d Compare May 30, 2025 04:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2025

aojea reviewed May 30, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2025

aojea reviewed May 30, 2025

View reviewed changes

Add generic buffer.TypedRingGrowing and shrinkable buffer.Ring #323

Are you sure you want to change the base?

Add generic buffer.TypedRingGrowing and shrinkable buffer.Ring #323

Uh oh!

Conversation

nojnhuh commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

nojnhuh commented Mar 25, 2025

Uh oh!

nojnhuh commented Apr 14, 2025

Uh oh!

Uh oh!

pohly commented May 20, 2025

Uh oh!

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

nojnhuh commented May 20, 2025

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

pohly commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2k May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2k May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2k May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2k commented May 28, 2025

Uh oh!

aojea commented May 29, 2025

Uh oh!

ash2k May 27, 2025 •

edited

Loading

ash2k May 27, 2025 •

edited

Loading

ash2k May 29, 2025 •

edited

Loading

aojea May 29, 2025 •

edited

Loading