From d3d871f156ce7d2aae32c00083ca3ee1c8ebb494 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Wed, 27 Sep 2023 20:20:16 -0400 Subject: [PATCH] catalog/lease: reduce lock contention on descVersionState 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 --- pkg/sql/catalog/lease/lease.go | 3 +-- pkg/sql/catalog/lease/lease_internal_test.go | 20 ++++++++++---------- pkg/sql/catalog/lease/name_cache.go | 20 +++++++++++--------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 77e391a06b15..1d1f5683d309 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -840,10 +840,9 @@ func (m *Manager) AcquireByName( return desc, nil } // Check if we have cached an ID for this name. - descVersion := m.names.get(ctx, parentID, parentSchemaID, name, timestamp) + descVersion, expiration := m.names.get(ctx, parentID, parentSchemaID, name, timestamp) if descVersion != nil { if descVersion.GetModificationTime().LessEq(timestamp) { - expiration := descVersion.getExpiration() // If this lease is nearly expired, ensure a renewal is queued. durationUntilExpiry := time.Duration(expiration.WallTime - timestamp.WallTime) if durationUntilExpiry < m.storage.leaseRenewalTimeout() { diff --git a/pkg/sql/catalog/lease/lease_internal_test.go b/pkg/sql/catalog/lease/lease_internal_test.go index 9a2daa7bc748..29962bc0eea6 100644 --- a/pkg/sql/catalog/lease/lease_internal_test.go +++ b/pkg/sql/catalog/lease/lease_internal_test.go @@ -396,7 +396,7 @@ CREATE TEMP TABLE t2 (temp int); for _, tableName := range []string{"t", "t2"} { tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, s.Codec(), "defaultdb", tableName) - lease := leaseManager.names.get( + lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), @@ -440,17 +440,17 @@ CREATE TABLE t.test (k CHAR PRIMARY KEY, v CHAR); } // Check that the cache has been updated. - if leaseManager.names.get( + if lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), "test", s.Clock().Now(), - ) != nil { + ); lease != nil { t.Fatalf("old name still in cache") } - lease := leaseManager.names.get( + lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), @@ -494,7 +494,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR); // Check the assumptions this tests makes: that there is a cache entry // (with a valid lease). - if lease := leaseManager.names.get( + if lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), @@ -509,7 +509,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR); leaseManager.ExpireLeases(s.Clock()) // Check the name no longer resolves. - if lease := leaseManager.names.get( + if lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), @@ -556,7 +556,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR); } // There is a cache entry. - lease := leaseManager.names.get( + lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), @@ -577,7 +577,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR); } // Check the name resolves to the new lease. - newLease := leaseManager.names.get(context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), tableName, s.Clock().Now()) + newLease, _ := leaseManager.names.get(context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), tableName, s.Clock().Now()) if newLease == nil { t.Fatalf("name cache doesn't contain entry for (%d, %s)", tableDesc.GetParentID(), tableName) } @@ -621,13 +621,13 @@ CREATE TABLE t.test (k CHAR PRIMARY KEY, v CHAR); tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, s.Codec(), "t", "test") // Check that we cannot get the table by a different name. - if leaseManager.names.get( + if lease, _ := leaseManager.names.get( context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), "tEsT", s.Clock().Now(), - ) != nil { + ); lease != nil { t.Fatalf("lease manager incorrectly found table with different case") } } diff --git a/pkg/sql/catalog/lease/name_cache.go b/pkg/sql/catalog/lease/name_cache.go index ee73a7266fb2..14cc943ac2a6 100644 --- a/pkg/sql/catalog/lease/name_cache.go +++ b/pkg/sql/catalog/lease/name_cache.go @@ -35,9 +35,10 @@ type nameCache struct { } // Resolves a (qualified) name to the descriptor's ID. -// Returns a valid descriptorVersionState for descriptor with that name, -// if the name had been previously cached and the cache has a descriptor -// version that has not expired. Returns nil otherwise. +// Returns a valid descriptorVersionState and expiration (hlc.Timestamp) +// for descriptor with that name, if the name had been previously cached +// and the cache has a descriptor version that has not expired. +// Returns nil (and empty timestamp) otherwise. // This method handles normalizing the descriptor name. // The descriptor's refcount is incremented before returning, so the caller // is responsible for releasing it to the leaseManager. @@ -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() - desc, ok := c.descriptors.GetByName( + var ok bool + desc, ok = c.descriptors.GetByName( parentID, parentSchemaID, name, ).(*descriptorVersionState) c.mu.RUnlock() if !ok { - return nil + return nil, expiration } expensiveLogEnabled := log.ExpensiveLogEnabled(ctx, 2) desc.mu.Lock() @@ -63,7 +65,7 @@ func (c *nameCache) get( // This get() raced with a release operation. Remove this cache // entry if needed. c.remove(desc) - return nil + return nil, hlc.Timestamp{} } defer desc.mu.Unlock() @@ -79,11 +81,11 @@ func (c *nameCache) get( // Expired descriptor. Don't hand it out. if desc.hasExpiredLocked(timestamp) { - return nil + return nil, hlc.Timestamp{} } desc.incRefCountLocked(ctx, expensiveLogEnabled) - return desc + return desc, desc.mu.expiration } func (c *nameCache) insert(desc *descriptorVersionState) {