Skip to content

Commit

Permalink
sql/catalog/descs: optimize access to the system db
Browse files Browse the repository at this point in the history
We tend to access the system database a lot and we never cache it in the
lease manager. Before this patch, we'd always copy and re-allocate a pair
of them. There's no need to do that.

```
name                                           old time/op    new time/op    delta
FlowSetup/vectorize=true/distribute=true-16       141µs ± 3%     132µs ± 4%  -6.35%  (p=0.000 n=19+18)
FlowSetup/vectorize=true/distribute=false-16      138µs ± 4%     129µs ± 3%  -6.80%  (p=0.000 n=19+18)
FlowSetup/vectorize=false/distribute=true-16      134µs ± 2%     124µs ± 4%  -7.55%  (p=0.000 n=20+17)
FlowSetup/vectorize=false/distribute=false-16     129µs ± 3%     120µs ± 3%  -6.98%  (p=0.000 n=20+18)

name                                           old alloc/op   new alloc/op   delta
FlowSetup/vectorize=true/distribute=true-16      38.1kB ± 2%    36.8kB ± 3%  -3.53%  (p=0.000 n=18+19)
FlowSetup/vectorize=true/distribute=false-16     36.2kB ± 0%    34.8kB ± 0%  -3.93%  (p=0.000 n=17+17)
FlowSetup/vectorize=false/distribute=true-16     42.6kB ± 0%    41.2kB ± 0%  -3.30%  (p=0.000 n=18+16)
FlowSetup/vectorize=false/distribute=false-16    41.0kB ± 0%    39.6kB ± 0%  -3.44%  (p=0.000 n=16+18)

name                                           old allocs/op  new allocs/op  delta
FlowSetup/vectorize=true/distribute=true-16         368 ± 0%       345 ± 0%  -6.25%  (p=0.000 n=16+16)
FlowSetup/vectorize=true/distribute=false-16        354 ± 0%       331 ± 0%  -6.50%  (p=0.000 n=18+18)
FlowSetup/vectorize=false/distribute=true-16        337 ± 0%       315 ± 1%  -6.69%  (p=0.000 n=19+19)
FlowSetup/vectorize=false/distribute=false-16       325 ± 0%       302 ± 0%  -7.08%  (p=0.000 n=17+18)
```

Release note: None
  • Loading branch information
ajwerner committed Oct 26, 2021
1 parent 31ccb1c commit bba05aa
Showing 1 changed file with 40 additions and 3 deletions.
43 changes: 40 additions & 3 deletions pkg/sql/catalog/descs/uncommitted_descriptors.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ package descs

import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
"github.com/cockroachdb/cockroach/pkg/util/iterutil"
"github.com/cockroachdb/errors"
Expand All @@ -23,8 +26,14 @@ import (
// uncommittedDescriptor is a descriptor that has been modified in the current
// transaction.
type uncommittedDescriptor struct {
mutable catalog.MutableDescriptor
immutable catalog.Descriptor

// mutable generally holds the descriptor as it was read from the database.
// In the rare case that this struct corresponds to a singleton which is
// added to optimize for special cases of system descriptors.
// It should be accessed through getMutable() which will construct a new
// value in cases where it is nil.
mutable catalog.MutableDescriptor
}

// GetName implements the catalog.NameEntry interface.
Expand All @@ -47,6 +56,16 @@ func (u uncommittedDescriptor) GetID() descpb.ID {
return u.immutable.GetID()
}

// getMutable is how the mutable descriptor should be accessed. It constructs
// a new descriptor in the case that this descriptor is a cached, in-memory
// singleton for a system descriptor.
func (u *uncommittedDescriptor) getMutable() catalog.MutableDescriptor {
if u.mutable != nil {
return u.mutable
}
return catalogkv.NewBuilder(u.immutable.DescriptorProto()).BuildExistingMutable()
}

var _ catalog.NameEntry = (*uncommittedDescriptor)(nil)

// uncommittedDescriptors is the data structure holding all
Expand Down Expand Up @@ -113,6 +132,7 @@ func (ud *uncommittedDescriptors) add(mut catalog.MutableDescriptor) (catalog.De
// checkOut checks out an uncommitted mutable descriptor for use in the
// transaction. This descriptor should later be checked in again.
func (ud *uncommittedDescriptors) checkOut(id descpb.ID) (catalog.MutableDescriptor, error) {
ud.maybeInitialize()
entry := ud.descs.GetByID(id)
if entry == nil {
return nil, errors.NewAssertionErrorWithWrappedErrf(
Expand All @@ -122,7 +142,7 @@ func (ud *uncommittedDescriptors) checkOut(id descpb.ID) (catalog.MutableDescrip

}
u := entry.(*uncommittedDescriptor)
return u.mutable, nil
return u.getMutable(), nil
}

// checkIn checks in an uncommitted mutable descriptor that was previously
Expand Down Expand Up @@ -170,6 +190,7 @@ func maybeRefreshCachedFieldsOnTypeDescriptor(

// getByID looks up an uncommitted descriptor by ID.
func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor {
ud.maybeInitialize()
entry := ud.descs.GetByID(id)
if entry == nil {
return nil
Expand All @@ -187,6 +208,7 @@ func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor {
func (ud *uncommittedDescriptors) getByName(
dbID descpb.ID, schemaID descpb.ID, name string,
) (hasKnownRename bool, desc catalog.Descriptor) {
ud.maybeInitialize()
// Walk latest to earliest so that a DROP followed by a CREATE with the same
// name will result in the CREATE being seen.
if got := ud.descs.GetByName(dbID, schemaID, name); got != nil {
Expand All @@ -202,9 +224,10 @@ func (ud *uncommittedDescriptors) getByName(
func (ud *uncommittedDescriptors) iterateNewVersionByID(
fn func(entry catalog.NameEntry, originalVersion lease.IDVersion) error,
) error {
ud.maybeInitialize()
return ud.descs.IterateByID(func(entry catalog.NameEntry) error {
mut := entry.(*uncommittedDescriptor).mutable
if mut.IsNew() || !mut.IsUncommittedVersion() {
if mut == nil || mut.IsNew() || !mut.IsUncommittedVersion() {
return nil
}
return fn(entry, lease.NewIDVersionPrev(mut.OriginalName(), mut.OriginalID(), mut.OriginalVersion()))
Expand All @@ -214,6 +237,7 @@ func (ud *uncommittedDescriptors) iterateNewVersionByID(
func (ud *uncommittedDescriptors) iterateImmutableByID(
fn func(imm catalog.Descriptor) error,
) error {
ud.maybeInitialize()
return ud.descs.IterateByID(func(entry catalog.NameEntry) error {
return fn(entry.(*uncommittedDescriptor).immutable)
})
Expand Down Expand Up @@ -262,3 +286,16 @@ func (ud *uncommittedDescriptors) hasUncommittedTypes() (has bool) {
})
return has
}

var systemUncommittedDatabase = &uncommittedDescriptor{
immutable: dbdesc.NewBuilder(systemschema.SystemDB.DatabaseDesc()).
BuildImmutableDatabase(),
// Note that the mutable field is left as nil. We'll generate a new
// value lazily when this is needed, which ought to be exceedingly rare.
}

func (ud *uncommittedDescriptors) maybeInitialize() {
if ud.descs.Len() == 0 {
ud.descs.Upsert(systemUncommittedDatabase)
}
}

0 comments on commit bba05aa

Please sign in to comment.