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

kvserver: cpu spike during import due to split stats computation #122309

Closed
kvoli opened this issue Apr 12, 2024 · 1 comment · Fixed by #122824
Closed

kvserver: cpu spike during import due to split stats computation #122309

kvoli opened this issue Apr 12, 2024 · 1 comment · Fixed by #122824
Assignees
Labels
branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker O-testcluster Issues found or occurred on a test cluster, i.e. a long-running internal cluster P-1 Issues/test failures with a fix SLA of 1 month T-kv KV Team
Projects

Comments

@kvoli
Copy link
Collaborator

kvoli commented Apr 12, 2024

Describe the problem

High CPU utilization during an import, which ranges are being split and scattered. The CPU is attributed to AdminSplitRequest computing the user MVCC stats of the (to be) LHS range:

userOnlyLeftStats, err = rditer.ComputeStatsForRangeUserOnly(

image

image

To Reproduce

See (internal) thread.

Expected behavior

CPU doesn't spike beyond a nominal 10-15%.

Additional data / screenshots

There are profiles and cluster information linked in the above (internal) thread.

Environment:

  • CockroachDB version V24.1.0-ALPHA.5-DEV-C43F54CDDE5B7578F4A0CA61DE41463F0D690993

Jira issue: CRDB-37797

@kvoli kvoli added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-kv KV Team O-testcluster Issues found or occurred on a test cluster, i.e. a long-running internal cluster labels Apr 12, 2024
@blathers-crl blathers-crl bot added this to Incoming in KV Apr 12, 2024
@kvoli kvoli added P-1 Issues/test failures with a fix SLA of 1 month branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 labels Apr 12, 2024
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Apr 12, 2024
@kvoli kvoli changed the title kvserver: cpu spike during import due to split stats recomputation kvserver: cpu spike during import due to split stats computation Apr 12, 2024
@miraradeva
Copy link
Contributor

@kvoli and I looked at this together and it does seem likely that the new split behavior, introduced in #119894, is causing instability in two ways (see below). The precondition here is that the import is issuing tens (we see 60 in the logs for n11) of manual splits on the same non-empty range. In the new split behavior, each split first re-computes the MVCC stats for the range (to prevent stats from drifting across successive splits), and then computes the stats for the user data on the left hand side (to pass to the split trigger).

  1. Issuing multiple RecomputeStats requests concurrently for the same range causes latch contention on the range descriptor. This results in many failed splits because many of the RecomputeStats request wait for a while and the split times out.
Screenshot 2024-04-15 at 2 12 23 PM
  1. Computing the LHS user-data stats tens of times concurrently for the same range can easily account for the CPU utilization we see. If the range is half-full, at 256MB, 60 such scans would amount to scanning over 15GB of data. The reason this was not an issue with the old accurate-stats splits is because the stats computation would happen in the split trigger while holding latches; acquiring latches would naturally sequence these splits, and not cause a CPU spike.

To confirm the above, we can disable estimated-stats splits (kv.split.estimated_mvcc_stats.enabled) and ensure there are no CPU spikes. To address 1, we can try disabling stats re-computation (kv.split.mvcc_stats_recomputation.enabled). For 2, we would need to change the current behavior. One option is to revert back to accurate-stats computation for manual AdminSplit requests. That should be ok because the main use cases for the new estimate-stats splits were mostly around size-based and load-based splits.

miraradeva added a commit to miraradeva/cockroach that referenced this issue Apr 22, 2024
Manual splits issued via AdminSplit are used in bulk operations, like
import, and tests to split many ranges out of the same original range.
Pre-computing the LHS user stats for each of these ranges concurrently
causes CPU spikes and split slowness; issuing repeated RecomputeStats
requests for the same range contributes even more and can cause
contention on the range descriptor.

Estimating MVCC stats during a split is an improvement targeted at
size-based splits, so in this patch we revert the manual-split behavior
back to computing accurate MVCC stats.

Fixes: cockroachdb#122309

Release note: None
craig bot pushed a commit that referenced this issue Apr 23, 2024
122824: kvserver: opt manual splits out of estimated MVCC stats r=miraradeva a=miraradeva

Manual splits issued via AdminSplit are used in bulk operations, like import, and tests to split many ranges out of the same original range. Pre-computing the LHS user stats for each of these ranges concurrently causes CPU spikes and split slowness; issuing repeated RecomputeStats requests for the same range contributes even more and can cause contention on the range descriptor.

Estimating MVCC stats during a split is an improvement targeted at size-based splits, so in this patch we revert the manual-split behavior back to computing accurate MVCC stats.

Fixes: #122309

Release note: None

122908: roachtest: use time.Duration instead of int64 for latency measurement r=rafiss a=rafiss

This makes the time printed in a more readable way in logs.

Epic: None
Release note: None

122926: roachtest: wait for 3x replication before chaos in multiregion/system-database r=rafiss a=rafiss

The test could previously kill nodes before replication was complete, so
we could lose quorum. Now we make sure replication is completed first.

fixes #122742
Release note: None

Co-authored-by: Mira Radeva <mira@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
@craig craig bot closed this as completed in 4dbf9a5 Apr 23, 2024
miraradeva added a commit that referenced this issue Apr 24, 2024
Manual splits issued via AdminSplit are used in bulk operations, like
import, and tests to split many ranges out of the same original range.
Pre-computing the LHS user stats for each of these ranges concurrently
causes CPU spikes and split slowness; issuing repeated RecomputeStats
requests for the same range contributes even more and can cause
contention on the range descriptor.

Estimating MVCC stats during a split is an improvement targeted at
size-based splits, so in this patch we revert the manual-split behavior
back to computing accurate MVCC stats.

Fixes: #122309

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-24.1 Used to mark GA and release blockers and technical advisories for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker O-testcluster Issues found or occurred on a test cluster, i.e. a long-running internal cluster P-1 Issues/test failures with a fix SLA of 1 month T-kv KV Team
Projects
KV
Incoming
Development

Successfully merging a pull request may close this issue.

2 participants