kvflowcontrol: performance optimizations in tracker.go

[aadityasondhi]

This patch makes 2 main performance optimizations in the tracker
codepath:

1. Replace the trackedList with a struct that points to a list. This
   avoids map assignments each time anything is appended to the list.
2. We use a sync.Pool for tracked items to avoid frequent allocations
   and gc for the objects.

Informs #104154.

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi and @irfansharif)

---

pkg/kv/kvserver/kvflowcontrol/kvflowtokentracker/tracker.go line 72 at r1 (raw file):

type trackedList struct {
	items []*tracked
}

The

trackedM map[admissionpb.WorkPriority]*trackedList

change is fine in terms of avoiding unnecessary map assignments.

But I don't quite see why this trackedList data-structure is the right one. It used to be []tracked and I assume the problem there was slice reallocation since we push into the back and pop from the front and keep re-allocating. We haven't fixed that part and we've only reduced the cost of slice reallocation by storing an 8 byte pointer instead of a 24 byte struct (tracked). I think we should abstract out the trackedList into a separate data-structure that allows pushing to the back and peeking and popping from the front, by treating the underlying slice ([]tracked) in the implementation as a circular buffer.

One concern with such a circular buffer is that it could grow very large when there was a burst of sends on a replica, and would not shrink when the replica had less traffic. A way to fix this is to keep a small amount of extra book-keeping -- say the max length of what was tracked over the last N pushes where N is the current capacity of the circular buffer. And if that max length is < N/2, then reallocate to a buffer that is N/2. That is, both dynamically grow and shrink the circular buffer.

Also, can you add a Benchmark so that we can compare the memory allocations for the existing solution and what is in this commit.

[aadityasondhi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @sumeerbhola)

---

pkg/kv/kvserver/kvflowcontrol/kvflowtokentracker/tracker.go line 72 at r1 (raw file):

Previously, sumeerbhola wrote…

The

trackedM map[admissionpb.WorkPriority]*trackedList

change is fine in terms of avoiding unnecessary map assignments.

But I don't quite see why this trackedList data-structure is the right one. It used to be []tracked and I assume the problem there was slice reallocation since we push into the back and pop from the front and keep re-allocating. We haven't fixed that part and we've only reduced the cost of slice reallocation by storing an 8 byte pointer instead of a 24 byte struct (tracked). I think we should abstract out the trackedList into a separate data-structure that allows pushing to the back and peeking and popping from the front, by treating the underlying slice ([]tracked) in the implementation as a circular buffer.

One concern with such a circular buffer is that it could grow very large when there was a burst of sends on a replica, and would not shrink when the replica had less traffic. A way to fix this is to keep a small amount of extra book-keeping -- say the max length of what was tracked over the last N pushes where N is the current capacity of the circular buffer. And if that max length is < N/2, then reallocate to a buffer that is N/2. That is, both dynamically grow and shrink the circular buffer.

Also, can you add a Benchmark so that we can compare the memory allocations for the existing solution and what is in this commit.

You are right, my change actually made the mem allocs worse. 0.4% -> 0.7% in heap profiles. My guess is that this is causing the additional allocs:

if _, ok := dt.trackedM[pri]; !ok {
		dt.trackedM[pri] = &trackedList{}
	}

I like the idea of the circular buffer. I might experiment a little more before going that route. Since pri is only one of a handful constants. Maybe deleting an empty trackedList might not be smart. We could just keep it around. I will try that and report back on the results.

[aadityasondhi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @sumeerbhola)

---

pkg/kv/kvserver/kvflowcontrol/kvflowtokentracker/tracker.go line 72 at r1 (raw file):

Previously, aadityasondhi (Aaditya Sondhi) wrote…

You are right, my change actually made the mem allocs worse. 0.4% -> 0.7% in heap profiles. My guess is that this is causing the additional allocs:

if _, ok := dt.trackedM[pri]; !ok {
		dt.trackedM[pri] = &trackedList{}
	}

I like the idea of the circular buffer. I might experiment a little more before going that route. Since pri is only one of a handful constants. Maybe deleting an empty trackedList might not be smart. We could just keep it around. I will try that and report back on the results.

Here is a doc tracking the tests results I am running: https://docs.google.com/document/d/1VRbbHMHmc9rKLk9e739rjWSs4RgG8weVR15_DqVTZ2k/edit

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi and @irfansharif)

---

pkg/kv/kvserver/kvflowcontrol/kvflowtokentracker/tracker.go line 72 at r1 (raw file):

Maybe deleting an empty trackedList might not be smart. We could just keep it around. I will try that and report back on the results.

I suspect the common case is a roughly stable count of tracked items for a priority. In that case the current sub-slicing approach can't avoid re-allocation.