catalog/lease: reduce lock contention on descVersionState #111397
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/catalog/lease/name_cache.go line 38 at r1 (raw file):
// Resolves a (qualified) name to the descriptor's ID. // Returns a valid descriptorVersionState for descriptor with that name,
small nit: could you update this comment to reflect your change
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
force-pushed
the
leasingRegression
branch
from
b0a2175
to
318b193
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/name_cache.go line 38 at r1 (raw file):
Previously, annrpom (annie pompa) wrote…
small nit: could you update this comment to reflect your change
Done.
| @@ -47,14 +48,15 @@ func (c *nameCache) get( | |||
| parentSchemaID descpb.ID, | |||
| name string, | |||
| timestamp hlc.Timestamp, | |||
| ) *descriptorVersionState { | |||
| ) (desc *descriptorVersionState, expiration hlc.Timestamp) { | |||
| c.mu.RLock() | |||
There was a problem hiding this comment.
as described in As described in #109443 (comment), RWMutex performs badly under high concurrency. Should we just change to a normal mutex?
There was a problem hiding this comment.
Let me try this out and see how it behaves without this change.
There was a problem hiding this comment.
No, it's similar to the old one:
BenchmarkLeaseAcquireByNameCached-24 1624814 734.4 ns/op
Also, there are no writes to the name cache in this scenario, so it should hit the fast path on the RWMutex (outside of system table activity).
|
@rafiss TFTR! bors r+ |
|
Build succeeded: |
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