Reducing the size of the struct like we've done is good, but the total count of objects allocated is important which can cause undue lot of pressure on the Go GC. It seems that for every *ReplicaStats object we have 6 underlying *replicaStatsRecord objects.

Also for every *Replica on a store, we have 6 *ReplicaStats objects.

So that's a multiplier of 6*6=36 objects per replica. Could we get this down to just one object allocated per-replica? I suspect that's the bottleneck more than the size of each object itself.

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

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

pkg/kv/kvserver/replicastats/replica_stats.go line 104 at r1 (raw file):

// mergeReplicaStatsRecords combines two records and returns a new record with
// the merged data. When this is called with nil records, a nil record is

update the comment? at least replace nil with "inactive" or something like that.

pkg/kv/kvserver/replicastats/replica_stats.go line 107 at r1 (raw file):

// returned; otherwise a new record instead.
func (rsr *replicaStatsRecord) mergeReplicaStatsRecords(other *replicaStatsRecord) {
	if !rsr.active && !other.active {

can this ever happen? I think no but maybe I'm missing something.

pkg/kv/kvserver/replicastats/replica_stats.go line 115 at r1 (raw file):

	}

	if !other.active {

I think these 2 if statements are also not needed? because we will always reset when deactivating?

Code quote:

	if !rsr.active {
		rsr.reset()







	}

	if !other.active {
		other.reset()
	}

pkg/kv/kvserver/replicastats/replica_stats.go line 120 at r1 (raw file):

	rsr.sum += other.sum
	rsr.active = true

btw in the old behavior we "activated" other.. right? so this is different. this new behavior makes more sense to me though.

pkg/kv/kvserver/replicastats/replica_stats.go line 210 at r1 (raw file):

		if !rs.Mu.records[i].active {
			other.Mu.records[i].reset()
			other.Mu.records[i].active = false

reset() will already set active to false, right? so maybe drop this line?

can we simplify things by having activate() and deactivate() or something like that? I feel like I need to track reset() together with active which is error prone.

I agree we should reconsider the structure - however I'm opting to not move to this single object model yet. I don't think it would address the OOM issue that motivated this change and I would rather put in a smaller fix here.

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

pkg/kv/kvserver/replicastats/replica_stats.go line 104 at r1 (raw file):

Updated.

pkg/kv/kvserver/replicastats/replica_stats.go line 107 at r1 (raw file):

This can happen - it's possible that either has only gone through < 6 windows, in which case they would be inactive.

The logic is bloated however, I've updated to make this clearer.

pkg/kv/kvserver/replicastats/replica_stats.go line 115 at r1 (raw file):

Updated - see above.

pkg/kv/kvserver/replicastats/replica_stats.go line 210 at r1 (raw file):

Good idea. Updated.

consider filing a short issue about this so we don't forget (unless you can send the pr soon).

Reviewed 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @kvoli)

pkg/kv/kvserver/replicastats/replica_stats.go line 107 at r1 (raw file):

thanks!

pkg/kv/kvserver/replicastats/replica_stats.go line 78 at r3 (raw file):

		syncutil.Mutex
		idx int
		// the window records are initialized once, then each window is reused

s/the/The/ :)
and a double space on the next line (I missed those!)

thanks for the comment btw.

Code quote:

the

pkg/kv/kvserver/replicastats/replica_stats.go line 103 at r3 (raw file):

	// values to avoid churning memory. We could also delete elements that
	// haven't been seen in a while, however it makes no difference as the
	// memory wouldn't be released (https://github.com/golang/go/issues/20135).

nice!

Code quote:

	// haven't been seen in a while, however it makes no difference as the
	// memory wouldn't be released (https://github.com/golang/go/issues/20135).
	// To protect against a memory leak, where many unused localities
	// accumulate, create a new map if the number of unused localities was
	// greater than 1.

pkg/kv/kvserver/replicastats/replica_stats.go line 240 at r3 (raw file):

			continue
		}
		// Otherwise, active the rhs record and split the request count between

s/active/activate/ probably?

Code quote:

active 

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

pkg/kv/kvserver/replicastats/replica_stats.go line 107 at r1 (raw file):

updated

pkg/kv/kvserver/replicastats/replica_stats.go line 120 at r1 (raw file):

yeah - changed to be more logical.

pkg/kv/kvserver/replicastats/replica_stats.go line 78 at r3 (raw file):

fixed.

pkg/kv/kvserver/replicastats/replica_stats.go line 240 at r3 (raw file):

updated.