util/quotapool: clean up some commentary, invert Update logic

[ajwerner]

The Merge method was clunky and required type switched. Instead, the
quantity being used to do the update should know the type of the resource.

Also, renames the QuotaPool to AbstractPool.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Have you considered an AbstractPool.Update primitive that just takes a func? That way we won't have to box things just to call a method. Well, at least if everything gets inlined, which I'm hoping (if not, we could have a Begin and End pair of primitives).

[RaduBerinde]

Some motivation for that suggestion: the top level method won't look as clean, but that cleanliness comes at a high cost - you have to understand the interface that you're passing, and you have to mess around with pools. The code of the layer above is also more opaque to a reader compared to code that performs the necessary update right there.

[ajwerner]

Have you considered an AbstractPool.Update primitive that just takes a func? That way we won't have to box things just to call a method. Well, at least if everything gets inlined, which I'm hoping (if not, we could have a Begin and End pair of primitives).

Done! Turns out they don't even need to be inlined (the -m flag doesn't say anything about inline that code) to not escape (pkg/util/quotapool/intpool.go:296:14: func literal does not escape). When this code was first written, I don't think that was the case. This is great!

[RaduBerinde]

Awesome! Is whatever the func binds (eg readBytes) not escaping either?

[ajwerner]

Awesome! Is whatever the func binds (eg readBytes) not escaping either?

Nope pkg/kv/kvserver/tenantrate/limiter.go:129:15: func literal does not escape.

[RaduBerinde]

Great!!

It would be nice to have a benchmark for one of the more used limiters. I can add that separately though.

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

[ajwerner]

There's benchmarks. Let me run them.

[ajwerner]

TFTR! Seems like noise:

name                                          old time/op  new time/op  delta
IntPool-24                                     178ns ± 0%   179ns ± 1%    ~     (p=0.942 n=9+8)
ConcurrentIntPool/workers=1,quota=1-24         302ns ± 7%   307ns ±10%    ~     (p=0.647 n=9+10)
ConcurrentIntPool/workers=2,quota=2-24         342ns ±11%   358ns ± 4%  +4.59%  (p=0.033 n=10+10)
ConcurrentIntPool/workers=8,quota=4-24        4.01µs ± 2%  3.88µs ± 3%  -3.06%  (p=0.001 n=9+10)
ConcurrentIntPool/workers=128,quota=4-24      4.30µs ± 3%  4.12µs ± 1%  -4.17%  (p=0.000 n=10+9)
ConcurrentIntPool/workers=512,quota=128-24    4.45µs ± 3%  4.44µs ± 4%    ~     (p=0.985 n=9+10)
ConcurrentIntPool/workers=512,quota=513-24     778ns ± 2%   762ns ± 1%  -2.09%  (p=0.000 n=8+9)
ConcurrentIntPool/workers=512,quota=511-24     801ns ±10%   774ns ± 3%    ~     (p=0.093 n=9+8)
ConcurrentIntPool/workers=1024,quota=4-24     4.65µs ± 6%  4.47µs ± 5%    ~     (p=0.052 n=10+10)
ConcurrentIntPool/workers=1024,quota=4096-24   787ns ± 3%   765ns ± 1%  -2.78%  (p=0.000 n=7+9)
IntPoolFunc-24                                 219ns ± 2%   221ns ± 1%  +1.05%  (p=0.009 n=10+10)

bors r=RaduBerinde

[RaduBerinde]

Cool! Thanks!

I will rebase my other PR on top of this and try to use RateLimiter instead of the abstract pool.

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[RaduBerinde]

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

	rl.metrics.readBytesAdmitted.Inc(readBytes)
	rl.qp.Update(func(res quotapool.Resource) (shouldNotify bool) {
		rb := res.(*tokenBuckets)

Random question, but don't we need to update() here?

If a long time has passed since the last update, the reduction won't matter because next time we update() we'll be at the burst limit in either case. Well, maybe for this particular usage it's fine since we're accounting for something that happened at or before the last update anyway, but it seems a little strange.

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

Previously, RaduBerinde wrote…

Random question, but don't we need to update() here?

If a long time has passed since the last update, the reduction won't matter because next time we update() we'll be at the burst limit in either case. Well, maybe for this particular usage it's fine since we're accounting for something that happened at or before the last update anyway, but it seems a little strange.

If somebody was blocked waiting, they already have a timer. See the comment above the return. 🤷

[RaduBerinde]

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

Previously, ajwerner wrote…

If somebody was blocked waiting, they already have a timer. See the comment above the return. 🤷

Consider this:

• lastUpdated is a while in the past (say twice the amount of time it takes to fully replenish the bucket)
• we get a RecordRead with a significant size (say the entire burst limit). We subtract the amount but do not update anything else.
• immediately we get a new request for quota. We update, and because enough time has passed (even if the previous step put us in debt), our bucket is full after updating. The request goes through.

Compare with:

• we get the RecordRead and we update the bucket (it is now full) and subtract the amount; bucket is now empty.
• immediately we get a new request for quota. The bucket was very recently updated so it's still empty. We have to wait.

With the current implementation, we can get anything in-between these two depending on what update() events may have happened in-between, even if they didn't make a significant difference in terms of tokens.

I realize this is a bit nitpicky - I'm not really worried about any actual practical repercussions of this difference. I'm asking because in the refactoring I proposed in #62783, I am updating before adjusting and wanted to run that by you.

[sumeerbhola]

this looks nice!

Reviewed 1 of 5 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

	rl.metrics.readBytesAdmitted.Inc(readBytes)
	rl.qp.Update(func(res quotapool.Resource) (shouldNotify bool) {
		rb := res.(*tokenBuckets)

nit: do we even need the res parameter? The limiter created tokenBuckets and the quotaPool so it knows the underlying resource.

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: do we even need the res parameter? The limiter created tokenBuckets and the quotaPool so it knows the underlying resource.

Interesting. We’d need to keep a reference to it on the *limiter struct and then some comment saying that it should only be accessed from underneath the quota pool. Is that better?

The pattern of having users of libraries which provide data structures type asserting to get their implementation out it pretty common in go. Consider https://pkg.go.dev/github.com/google/btree#Item

---

pkg/kv/kvserver/tenantrate/limiter.go, line 130 at r1 (raw file):

Previously, RaduBerinde wrote…

Consider this:

• lastUpdated is a while in the past (say twice the amount of time it takes to fully replenish the bucket)
• we get a RecordRead with a significant size (say the entire burst limit). We subtract the amount but do not update anything else.
• immediately we get a new request for quota. We update, and because enough time has passed (even if the previous step put us in debt), our bucket is full after updating. The request goes through.

Compare with:

• we get the RecordRead and we update the bucket (it is now full) and subtract the amount; bucket is now empty.
• immediately we get a new request for quota. The bucket was very recently updated so it's still empty. We have to wait.

With the current implementation, we can get anything in-between these two depending on what update() events may have happened in-between, even if they didn't make a significant difference in terms of tokens.

I realize this is a bit nitpicky - I'm not really worried about any actual practical repercussions of this difference. I'm asking because in the refactoring I proposed in #62783, I am updating before adjusting and wanted to run that by you.

Good point. Makes sense.