contention: add serialization of the contention registry

[yuzefovich]

contention: add serialization of the contention registry

This commit adds an ability to serialize the contention registry that
will be necessary to surface the contention information up from the
backend.

Addresses: #57114.

Release note: None

contention: merge serialized representations of registries

This commit adds a method to merge the serialized representation of
contention registries into one which will be needed to get a global
contention view.

Note that the merge operation respects the same constants that limit the
size of the registry. Additionally, the serialized representations now
respect the following orderings:

• on the highest level, all IndexContentionEvents objects are ordered
  according to their importance (achieved by an explicit sort)
• on the middle level, all SingleKeyContention objects are ordered by their
  keys (achieved by using the ordered cache)
• on the lowest level, all SingleTxnContention objects are ordered by the
  frequency of the occurrence (achieved by an explicit sort).
  The same guarantees are kept for the merge operation too.

Release note: None

[cockroach-teamcity]

This change is 

[asubiotto]

Nice!

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

---

pkg/sql/contentionpb/contention.go, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

contention.go doesn't seem like a good name for this file (it's too general), any ideas on something better?

---

pkg/sql/contentionpb/contention.proto, line 63 at r1 (raw file):

  // Counts is a 1-to-1 mapping with TxnIDs describing the number of times the
  // corresponding transaction was encountered.

Makes more sense to me to put txn_id and count in a single repeated message rather than just stating that there should be a 1-1 mapping.

[yuzefovich]

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

---

pkg/sql/contentionpb/contention.go, line 1 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

contention.go doesn't seem like a good name for this file (it's too general), any ideas on something better?

I think that we tend to name such files exactly as the proto file that defines messages for which we're adding String() methods, the proto file is named contention and I don't know if there is a better name for it.

---

pkg/sql/contentionpb/contention.proto, line 63 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Makes more sense to me to put txn_id and count in a single repeated message rather than just stating that there should be a 1-1 mapping.

Done.

[yuzefovich]

Added another commit to merge two representations. Let me know if you prefer to have a separate PR for it or whether I should squash the two commits (I'm keeping them separately for the ease of reviewing).

[yuzefovich]

The open question is whether we want to maintain the ordering of some things - in the registry, we are keeping []SingleKeyContention ordered by Key, but other things (tableID/indexID pairs and SingleTxnContention things are not ordered). Thoughts here?

[asubiotto]

Good point cc @awoods187. Might make sense to order the top level by table ID/index ID pair and the single txn contention by the number of times contended. I think we should do this in a separate PR and switch out the unorderedCache for orderedCache. Not sure why I didn't think of the ordering here but it seems like it'd be nice.

Reviewed 1 of 6 files at r1, 5 of 5 files at r2, 4 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/contention/registry.go, line 260 at r3 (raw file):

// contain contention events on the same index (i.e. with the same
// (tableID, indexID) pair).
func MergeSerializedRegistries(

I think there are two main concerns that we need to resolve:

1. In-memory registries are updated and evict using an LRU policy. I think we need to limit the size of the merged registries, but instead of using an LRU policy, this should be based on a notion of most important contention events (possibly defined by the number of contention events). i.e., we should probably respect the max keys constant (let it be k) when merging and keep the top k keys in a given index (similarly with indexes and transactions).
2. We need to maintain key ordering. This is sort of like the TimeMap interview question 😂 Easiest but most allocations is probably to expand the left to take all right elements then do a merge sort, find the top k using a min heap to track indices like the sorter and swap them, then truncate. Maybe we can do something cleverer, especially if we take into account that we'll want to merge n registries, where n is the number of nodes. At the same time, we probably don't need to overoptimize this.

---

pkg/sql/contentionpb/contention.go, line 1 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think that we tend to name such files exactly as the proto file that defines messages for which we're adding String() methods, the proto file is named contention and I don't know if there is a better name for it.

Got it, the fact that it's in contentionpb (missed that) makes it ok.

[awoods187]

Might make sense to order the top level by table ID/index ID pair and the single txn contention by the number of times contended.

I agree. A hierarchy of table/index first and contention second makes sense to me. Just to confirm, can i see an example?

[yuzefovich]

Updated the second commit to maintain the desired ordering and to respect the same constants.

I don't think that we can replace the unordered caches because we need to be querying them by tableID/indexID pair for internalCache (and not by the "importance" of IndexContentionEvents) and by txnID for txnCache (and not by their occurrence), so instead of using the ordered caches I added explicit sorts for those two.

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

---

pkg/sql/contention/registry.go, line 260 at r3 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think there are two main concerns that we need to resolve:

1. In-memory registries are updated and evict using an LRU policy. I think we need to limit the size of the merged registries, but instead of using an LRU policy, this should be based on a notion of most important contention events (possibly defined by the number of contention events). i.e., we should probably respect the max keys constant (let it be k) when merging and keep the top k keys in a given index (similarly with indexes and transactions).
2. We need to maintain key ordering. This is sort of like the TimeMap interview question 😂 Easiest but most allocations is probably to expand the left to take all right elements then do a merge sort, find the top k using a min heap to track indices like the sorter and swap them, then truncate. Maybe we can do something cleverer, especially if we take into account that we'll want to merge n registries, where n is the number of nodes. At the same time, we probably don't need to overoptimize this.

Updated the second commit to address both of these concerns.

---

pkg/sql/contentionpb/contention.go, line 1 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Got it, the fact that it's in contentionpb (missed that) makes it ok.

Done.

[yuzefovich]

Added a unit test for the invariants that are now maintained on the serialized representations.

[asubiotto]

I don't think that we can replace the unordered caches because we need to be querying them by tableID/indexID pair for internalCache (and not by the "importance" of IndexContentionEvents) and by txnID for txnCache (and not by their occurrence), so instead of using the ordered caches I added explicit sorts for those two.

Hmm I see. We can't move the count to the key, otherwise these would be different objects in the cache. I think sorts are fine. Wonder if we can do anything about this in the future.

Reviewed 6 of 6 files at r4, 6 of 6 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

// licenses/APL.txt.

package contention

nit: can we make this contention_test and move setSizeConstants to a test-only utils_test.go file?

---

pkg/sql/contention/registry_test.go, line 197 at r5 (raw file):

// - all three levels of objects satisfy the respective ordering
//   requirements.
func TestSerializedRegistryInvariants(t *testing.T) {

nice test

[yuzefovich]

Yeah. To me it doesn't look like a big concern because we have limits on the sizes of these caches in place, so we won't be sorting many items. Additionally, the comparisons for these sorts are cheap (unlike for the ordered key cache).

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

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: can we make this contention_test and move setSizeConstants to a test-only utils_test.go file?

We cannot move setSizeConstants into contention_test package without exporting the constants. Additionally, there is a minor complication that TestRegistryConcurrentAdds is accessing the unexported fields too. So I think we should leave everything as is.

---

pkg/sql/contention/registry_test.go, line 197 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nice test

Thanks!

[asubiotto]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We cannot move setSizeConstants into contention_test package without exporting the constants. Additionally, there is a minor complication that TestRegistryConcurrentAdds is accessing the unexported fields too. So I think we should leave everything as is.

I mean moving setSizeConstants to a utils_test.go file still in the contention package. Then this file's package could be contention_test and still be able to call out to SetSizeConstants.

[yuzefovich]

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

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I mean moving setSizeConstants to a utils_test.go file still in the contention package. Then this file's package could be contention_test and still be able to call out to SetSizeConstants.

Done.

[asubiotto]

Reviewed 6 of 6 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Done.

nit: not sure if you forgot to change this file's package to contention_test or chose not to. If you forgot, once you do that you'll have to export SetSizeConstants. Oh, I see that there are also the other private functions this test relies on. Could those also be moved to utils_test.go and exported for tests?

[yuzefovich]

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

---

pkg/sql/contention/registry_test.go, line 11 at r5 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: not sure if you forgot to change this file's package to contention_test or chose not to. If you forgot, once you do that you'll have to export SetSizeConstants. Oh, I see that there are also the other private functions this test relies on. Could those also be moved to utils_test.go and exported for tests?

Done.

I was misunderstanding your intention.

[asubiotto]

Reviewed 7 of 7 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto)

[yuzefovich]

TFTR!

bors r+

[yuzefovich]

bors r+

[craig]

Already running a review

[craig]

Build succeeded:

• GitHub CI (Cockroach)