Add generic buffer.TypedRingGrowing and shrinkable buffer.Ring #323
Conversation
k8s-ci-robot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nojnhuh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
buffer/ring_growing.go
Outdated
| // Ring is a dynamically-sized ring buffer which can grow and shrink as-needed. | ||
| // Not thread safe. | ||
| type Ring[T any] struct { | ||
| TypedRingGrowing[T] |
There was a problem hiding this comment.
Embedding this type here is convenient to "inherit" its methods, but I'm not sure we want this in the public API. Or maybe that's actually a feature? If users have a Ring and want to do a read such that the buffer definitely will not shrink, they could do r.TypedRingGrowing.ReadOne().
There was a problem hiding this comment.
That particular usage seems a bit far-fetched and not like something that developers will discover if they need it - we would need to explicitly document the possibility.
I understand the convenience, but I have been bitten by wrappers embedding the type that they wrap when extending the type with new methods. Everything compiles fine even if the wrapper would have to be updated to also wrap the new methods.
It's cleaner to not embed the underlying type and instead repeat all methods as calls of the underlying method.
| type Ring[T any] struct { | ||
| TypedRingGrowing[T] | ||
|
|
||
| normalSize int // Limits the size of the buffer that is kept for reuse. Read-only. |
There was a problem hiding this comment.
queue.FIFO is set up to not require any constructors like this, so users can initialize one directly as the zero value and get a valid, empty queue. One detail I couldn't quite iron out to enable that here is how to make the normalSize configurable. There it's hardcoded to 4 which seems reasonable for where that's used, but I see informers' notification buffers using RingGrowing are initialized to 1024, so shrinking all the way down to 4 could result in significantly more allocations. So if there isn't a single golden value for every use case, I think that value probably has to be fed in through a constructor (or possibly a method).
There was a problem hiding this comment.
I agree that a constructor parameter would be good. Two int parameters with no clear obvious ordering will be hard to read at call sites, so I prefer the config struct pattern.
We could use optional With* methods, but that raises the question whether they return a deep copy of the instance or completely new ones - let's not go there.
There was a problem hiding this comment.
Regarding the zero value Ring[T]: let's document it as NewRing() with zero initial size and zero normal size. Perhaps not very useful, but at least it's then documented.
There was a problem hiding this comment.
Added options structs and mentioned zero values in doc strings.
|
/cc @pohly |
Use unexported field for shrinkable Ring's RingGrowing
|
|
||
| // ReadOne reads (consumes) first item from the buffer if it is available, | ||
| // otherwise returns false. When the buffer has been totally consumed and has | ||
| // grown in size, it shrinks down to its initial size. |
There was a problem hiding this comment.
| // grown in size, it shrinks down to its initial size. | |
| // grown in size beyond its normal size, it shrinks down to its normal size again. |
I was wondering whether two different options are needed, but I can imaging that RingOptions{InitialSize: 8, NormalSize: 32} could make sense: we get some memory reuse for small problems with less than 8 entries max. If the problem space is larger, we hang on to more memory to deal with it.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a new type,
buffer.Ringwhich is abuffer.RingGrowingwith 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 ofqueue.FIFO(and its copy ink8s.io/dynamic-resource-allocation/internal/queue). Behavior of the existingbuffer.RingGrowingshould 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/cachewherebuffer.RingGrowingis currently in use without any changes to that package, and fork8s.io/kubernetes/pkg/scheduler/util/assumecacheandk8s.io/dynamic-resource-allocation/resourceslice/trackerreplacing its use ofqueue.FIFO: kubernetes/kubernetes@master...nojnhuh:kubernetes:typed-ring-buffer.Release note: