Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-23.1: kv: wait on latches on each key in reverse acquisition order #109370

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Aug 23, 2023

Backport 1/1 commits from #109349 on behalf of @nvanbenschoten.

/cc @cockroachdb/release


This commit allocates latch IDs from the top of the uint64 space and in reverse order. This is done to order latches in the tree on the same key in reverse order of acquisition. Doing so ensures that when we iterate over the tree and see a key with many conflicting latches, we visit the latches on that key in the reverse order that they will be released. In doing so, we minimize the number of open channels that we wait on (calls to waitForSignal) and minimize the number of goroutine scheduling points. This is important to avoid spikes in runnable goroutine after each request completes, which can negatively affect node health.

See experiments below.

Epic: None
Release note (performance improvement): The impact of high concurrency blind writes to the same key on goroutine scheduling latency was reduced.


Release justification: small performance and stability improvement.

This commit allocates latch IDs from the top of the uint64 space and in
reverse order. This is done to order latches in the tree on a same key
in reverse order of acquisition. Doing so ensures that when we iterate
over the tree and see a key with many conflicting latches, we visit the
latches on that key in the reverse order that they will be released. In
doing so, we minimize the number of open channels that we wait on (calls
to `waitForSignal`) and minimize the number of goroutine scheduling
points. This is important to avoid spikes in runnable goroutine after
each request completes, which can negatively affect node health.

Epic: None
Release note (performance improvement): The impact of high concurrency
blind writes to the same key on goroutine scheduling latency was reduced.
@blathers-crl blathers-crl bot requested a review from a team as a code owner August 23, 2023 21:27
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-109349 branch from e6d0a52 to 0f3f856 Compare August 23, 2023 21:27
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Aug 23, 2023
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-109349 branch from 85ee7f0 to 89ee660 Compare August 23, 2023 21:27
@blathers-crl
Copy link
Author

blathers-crl bot commented Aug 23, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl
Copy link
Author

blathers-crl bot commented Aug 23, 2023

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
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stamping the backport, but let's let this bake on master for a few weeks like you mentioned in your original PR.

@irfansharif irfansharif removed their request for review August 29, 2023 13:30
@nvanbenschoten nvanbenschoten merged commit 5812fec into release-23.1 Sep 20, 2023
@nvanbenschoten nvanbenschoten deleted the blathers/backport-release-23.1-109349 branch September 20, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants