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.

Sign up for GitHub

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

Jump to bottom

lease: regression in BenchmarkLeaseAcquireByNameCached from 23.1 #111094

Closed
yuzefovich opened this issue Sep 22, 2023 · 0 comments · Fixed by #111397
Closed

lease: regression in BenchmarkLeaseAcquireByNameCached from 23.1 #111094

yuzefovich opened this issue Sep 22, 2023 · 0 comments · Fixed by #111397
Assignees
@fqazi
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-performance Perf of queries or internals. Solution not expected to change functional behavior. regression Regression from a release. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@yuzefovich
Copy link
Member

yuzefovich commented Sep 22, 2023 •
edited
Loading

Comparing f13c021 and 2a23692

name                         old time/op    new time/op    delta
LeaseAcquireByNameCached-24     548ns ± 3%     696ns ± 9%  +26.98%  (p=0.000 n=10+10)

name                         old alloc/op   new alloc/op   delta
LeaseAcquireByNameCached-24     8.00B ± 0%     6.70B ±25%  -16.25%  (p=0.000 n=9+10)

name                         old allocs/op  new allocs/op  delta
LeaseAcquireByNameCached-24      0.00           0.00          ~     (all equal)

Interestingly, an automated run got 1500% in alloc/op increase (although the next automated run was in line with what I reproduced manually on gceworker).

Jira issue: CRDB-31758
@yuzefovich yuzefovich added regression Regression from a release. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Sep 22, 2023
@yuzefovich yuzefovich added the C-performance Perf of queries or internals. Solution not expected to change functional behavior. label Sep 22, 2023
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Sep 22, 2023
@williamkulju williamkulju added the branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 label Sep 22, 2023
@yuzefovich yuzefovich added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Sep 22, 2023
@fqazi fqazi self-assigned this Sep 26, 2023
fqazi added a commit to fqazi/cockroach that referenced this issue Sep 28, 2023
@fqazi
catalog/lease: reduce lock contention on descVersionState
b0a2175 
Recently a regression was noticed on master related to AcquireByName
being slower between builds. Using a profiler this was tracked down to
contention on descriptorVersionState's mutex.  To address this, this
patch avoids acquiring this lock twice in the hot path, which resolves
the regression.

After:   BenchmarkLeaseAcquireByNameCached-24    	 2378610	       451.9 ns/op
Before:  BenchmarkLeaseAcquireByNameCached-24    	 1493899	       727.9 ns/op
f13c021: BenchmarkLeaseAcquireByNameCached-24    	 2129446	       568.1 ns/op

Fixes: cockroachdb#111094

Release note: None
@fqazi fqazi mentioned this issue Sep 28, 2023
Merged
craig bot pushed a commit that referenced this issue Oct 2, 2023
@knz @fqazi
Merge #106118 #109415 #111397
0be8311 
106118: pkg/ui: emphasize "cluster virtualization" in human-visible surfaces r=yuzefovich a=knz

Informs #106068.
Epic: CRDB-29380

At this stage, we are focusing on strings and comments that are read by humans. A separate pass will take care of the method and variable names later.

Release note: None

109415: server, all: rename some cluster settings r=rafiss a=knz

Fixes #75520.
Epic: CRDB-28893

The following settings have been renamed, to emphasize how the initial drain wait is a *mandatory wait*, whereas the others are simply *timeouts*.

Release note (cli change): The following cluster settings have been renamed; the previous names remain available for
backward-compatibility.

| Previous name                         | New Name                                           |
|---------------------------------------|----------------------------------------------------|
| `server.shutdown.drain_wait`          | `server.shutdown.initial_wait`                     |
| `server.shutdown.lease_transfer_wait` | `server.shutdown.lease_transfer_iteration.timeout` |
| `server.shutdown.query_wait`          | `server.shutdown.queries.timeout`                  |
| `server.shutdown.connection_wait`     | `server.shutdown.transactions.timeout`              |
| `server.shutdown.jobs_wait`           | `server.shutdown.jobs.timeout`                     |

111397: catalog/lease: reduce lock contention on descVersionState r=fqazi a=fqazi

Recently, a regression was noticed on master related to AcquireByName being slower between builds. Using a profiler, this was tracked down to contention on descriptorVersionState's mutex.  To address this, this patch avoids acquiring this lock twice in the hot path, which resolves the regression.

After:   BenchmarkLeaseAcquireByNameCached-24    	 2378610	       451.9 ns/op
Before:  BenchmarkLeaseAcquireByNameCached-24    	 1493899	       727.9 ns/op
f13c021: BenchmarkLeaseAcquireByNameCached-24    	 2129446	       568.1 ns/op

Fixes: #111094

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
@craig craig bot closed this as completed in 318b193 Oct 2, 2023
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Oct 6, 2023
@fqazi
catalog/lease: reduce lock contention on descVersionState
d3d871f 
Recently, a regression was noticed on master related to AcquireByName
being slower between builds. Using a profiler, this was tracked down to
contention on descriptorVersionState's mutex.  To address this, this
patch avoids acquiring this lock twice in the hot path, which resolves
the regression.

After:   BenchmarkLeaseAcquireByNameCached-24    	 2378610	       451.9 ns/op
Before:  BenchmarkLeaseAcquireByNameCached-24    	 1493899	       727.9 ns/op
f13c021: BenchmarkLeaseAcquireByNameCached-24    	 2129446	       568.1 ns/op

Fixes: cockroachdb#111094

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fqazi fqazi

Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-performance Perf of queries or internals. Solution not expected to change functional behavior. regression Regression from a release. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
SQL Foundations
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

catalog/lease: reduce lock contention on descVersionState fqazi/cockroach
3 participants
@fqazi @yuzefovich @williamkulju