sql: add a minimum allocation field to bound accounts

[changangela]

This change adds on to the work done in #30924.

Currently, a bound account will reserve a small buffer of up to poolAllocationSize. When the account grows, this buffer is first used up before additional memory is reserved from the monitor. When the account shrinks, memory is released from this buffer to a minimum of poolAllocationSize. We want to be able to dynamically set a minimum allocated value in order to amortize the cost of constantly growing and shrinking a account. This field is different from poolAllocationSize in the
following ways:

• there is overhead introduced by a call to releaseBytes and if we are always "shrinking" the reserved buffer to a constant size of poolAllocationSize, this overhead becomes quite prominent. Instead, if we establish a minimum value for allocated = reserved + used, then the call to releaseBytes becomes much less frequent.
• in the case where we "grow" the account to above the minAllocated size, then we don't want the account to buffer up another chunk of minAllocated, instead it will reserve memory in conservative poolAllocationSize blocks.

This PR mainly adds the necessary documentation and comments that was not present in #30924.

Release note: None

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[changangela]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/util/mon/bytes_usage.go, line 661 at r1 (raw file):

// reserveBytes().
func (mm *BytesMonitor) releaseBytes(ctx context.Context, sz int64) {
	if sz == 0 {

Prior to #31191, adding this condition sped up the MergeJoinerBenchmark by ~50%. However, the same problem could be fixed by using the condition released > 0 instead of released >= 0. I originally added this check in as a precaution for users without thinking of the overhead. Since it doesn't provide any additional value specific to the current bytes_usage functions, I've removed it.

[changangela]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/util/mon/bytes_usage.go, line 454 at r2 (raw file):

	// minAllocated is a minimum allocated bytes size that the account reach
	// before being able to release bytes.
	minAllocated int64

Working on investigating the memory usage concerns with adding this extra field.

[knz]

Thanks for the additional insight. Please have a look at my remaining comment below.

With this additional insight I must say I am surprised by this change -- it looks to me that you could have achieved the same result in the distsql processor as follows:

1. request the minallocated bytes via a regular (unchanged) BoundAccount
2. create a sub-monitor (not account) in the processor with the boundaccount created at step 1 as reserved bytes
3. do memory accounting in the processor using the sub-monitor

I think this achieves the same, with a similar performance profile, and without changes to the mon package. What do you think?

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

---

pkg/util/mon/bytes_usage.go, line 447 at r2 (raw file):

// additional memory and parceling it out (see BoundAccount.reserved). The
// amount of additional memory is determined by the value of
// BoundAccount.minAllocated. By default, BoundAccount.minAllocated is set to 0.

What is the relationship between reserved, mentioned in the previous sentence, and minAllocated introduced here?

The phrasing currently reads like both fields are doing the same thing.

What does minAllocated add which is not already comprised in reserved? (Other way around: why is reserved not sufficient to set a minimum?)

[changangela]

Originally, I think we were going for a more general approach that wasn't specific to the distsql processor, but I think that using this sub-monitor approach makes sense to solve this problem makes sense. I'm thinking that using the sub-monitor would introduce more overhead than this approach because it will still have to talk back and forth with the BoundAccount to manipulate values. I'd be interested in trying it out and benchmarking it.

Reviewable status: complete! 0 of 0 LGTMs obtained

[changangela]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/util/mon/bytes_usage.go, line 447 at r2 (raw file):

Previously, knz (kena) wrote…

What is the relationship between reserved, mentioned in the previous sentence, and minAllocated introduced here?

The phrasing currently reads like both fields are doing the same thing.

What does minAllocated add which is not already comprised in reserved? (Other way around: why is reserved not sufficient to set a minimum?)

Done.