replica_rac2: optimize logTracker.admitted

[pav-kv]

Improve cache friendliness by checking the dirty and scheduling bits before resetting them. Cache the latest admitted vector to avoid computing it on every call. Instead, it is recomputed only if dirty bit is true.

Informs #128033

[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 

[pav-kv]

@sumeerbhola Tangentially to #132137, in case more optimizations are needed.

[sumeerbhola]

This specific doesn't seem relevant based on looking at profiles. I will look into more allocation reduction, and reduce mutex acquisitions and measure again.

Reviewable status: complete! 0 of 0 LGTMs obtained

[pav-kv]

It still separates the concerns a bit, should we merge? The slight micro-cost reduction is an optional bonus here.

[sumeerbhola]

Reviewed 1 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 42 at r1 (raw file):

// admitted returns the current admitted vector, and resets the dirty bit.
func (l *logTracker) admitted() rac2.AdmittedVector {

nit: prefer admittedAndResetDirty or something akin.

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 56 at r1 (raw file):

// allows the next logAdmitted call to return true and allow scheduling a Ready
// iteration again. This flow avoids unnecessary Ready scheduling events.
func (l *logTracker) admittedDirty() (av rac2.AdmittedVector, dirty bool) {

nit: prefer admittedIfDirtyAndResetDirty or something akin.

Even if it not fully self-explanatory, it causes readers to read the method comment.

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 59 at r1 (raw file):

	l.Lock()
	defer l.Unlock()
	l.scheduled = false

The thing I worry about optimizations that don't show up in profiles is that I can't judge the value of a branch reduction given out-of-order speculative execution in processors.

I don't generally have a good mental model here. https://johnnysswlab.com/how-branches-influence-the-performance-of-your-code-and-what-can-you-do-about-it/ is a decent article and concludes that one shouldn't try to reduce branches, and it is more important to be cache friendly. Unconditional writes violate the latter.

So if one were trying to micro-optimize this, I would suggest changing this to

if l.scheduled {
  l.scheduled = false
}

Same for the l.dirty = false in the admitted() method.

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 61 at r1 (raw file):

	l.scheduled = false
	if !l.dirty {
		return

nit: I believe the preference in CRDB code is to always do an explicit return with the values being returned.

[pav-kv]

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

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 42 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: prefer admittedAndResetDirty or something akin.

Not relevant in the new version of this PR.

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 56 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: prefer admittedIfDirtyAndResetDirty or something akin.

Even if it not fully self-explanatory, it causes readers to read the method comment.

Not relevant in the new version of this PR.

---

pkg/kv/kvserver/kvflowcontrol/replica_rac2/log_tracker.go line 59 at r1 (raw file):

Previously, sumeerbhola wrote…

The thing I worry about optimizations that don't show up in profiles is that I can't judge the value of a branch reduction given out-of-order speculative execution in processors.

I don't generally have a good mental model here. https://johnnysswlab.com/how-branches-influence-the-performance-of-your-code-and-what-can-you-do-about-it/ is a decent article and concludes that one shouldn't try to reduce branches, and it is more important to be cache friendly. Unconditional writes violate the latter.

So if one were trying to micro-optimize this, I would suggest changing this to

if l.scheduled {
  l.scheduled = false
}

Same for the l.dirty = false in the admitted() method.

Done. Since both methods would be doing these conditionals anyway, I merged them back to be one method. Instead, I am now caching the latest admitted vector (see the new av field). The idea is to avoid computing it on every call, and instead do it only if dirty == true (since we're checking this bit anyway).

[sumeerbhola]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @pav-kv)

[pav-kv]

TFTR!

bors r=sumeerbhola

[craig]

Build succeeded:

• macos_amd64_build
• acceptance
• local_roachtest_fips
• check_generated_code
• macos_arm64_build
• lint
• unit_tests
• docker_image_amd64
• local_roachtest
• linux_arm64_build
• linux_amd64_build
• linux_amd64_fips_build
• windows_build
• examples_orms