sql/catalog/descs: implement interfaces on *DistSQLTypeResolver#72962
sql/catalog/descs: implement interfaces on *DistSQLTypeResolver#72962craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This commit updates `DistSQLTypeResolver` to implement all interfaces on a pointer receiver instead of a value receiver. Implementing interfaces on values is rarely the right choice, as it forces a heap allocation whenever the object is boxed into an interface, instead of just forcing the pointer onto the heap once (on its own or as part of a larger object) and then storing the pointer in the interface header. Before this commit, the use of value receivers was causing `HydrateTypeSlice` to allocate. Outside of cockroachdb#72798 and cockroachdb#72961, this was the single largest source of heap allocations in TPC-E. With those two PRs applied, `HydrateTypeSlice` was accounting for **2.30%** of total heap allocations in the workload: ``` ----------------------------------------------------------+------------- flat flat% sum% cum cum% calls calls% + context ----------------------------------------------------------+------------- 27722149 32.66% | github.com/cockroachdb/cockroach/pkg/sql/execinfra.(*ProcessorBase).InitWithEvalCtx /go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:790 27460002 32.36% | github.com/cockroachdb/cockroach/pkg/sql/colflow.(*vectorizedFlowCreator).setupFlow.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:1097 21266755 25.06% | github.com/cockroachdb/cockroach/pkg/sql/colflow.(*vectorizedFlowCreator).setupInput /go/src/github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:818 8421503 9.92% | github.com/cockroachdb/cockroach/pkg/sql/colfetcher.populateTableArgs /go/src/github.com/cockroachdb/cockroach/pkg/sql/colfetcher/cfetcher_setup.go:174 84870409 2.30% 2.30% 84870409 2.30% | github.com/cockroachdb/cockroach/pkg/sql/catalog/descs.DistSQLTypeResolver.HydrateTypeSlice /go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/dist_sql_type_resolver.go:134 ----------------------------------------------------------+------------- ``` With this change, the heap allocation in `HydrateTypeSlice` disappears. With these three PRs combined, the largest source of heap allocations in the workload is `context.WithValue`, like the Go gods intended. ``` ----------------------------------------------------------+------------- flat flat% sum% cum cum% calls calls% + context ----------------------------------------------------------+------------- 16723340 38.17% | github.com/cockroachdb/logtags.WithTags /go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/logtags/context.go:34 7405899 16.90% | google.golang.org/grpc/peer.NewContext /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/peer/peer.go:44 3910493 8.93% | google.golang.org/grpc.NewContextWithServerTransportStream /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/server.go:1672 3702950 8.45% | github.com/cockroachdb/cockroach/pkg/util/tracing.maybeWrapCtx /go/src/github.com/cockroachdb/cockroach/pkg/util/tracing/context.go:80 3560952 8.13% | google.golang.org/grpc/metadata.NewIncomingContext /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/metadata/metadata.go:152 3342479 7.63% | google.golang.org/grpc.newContextWithRPCInfo /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/rpc_util.go:791 2938326 6.71% | google.golang.org/grpc/internal/credentials.NewRequestInfoContext /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/internal/credentials/credentials.go:29 1387235 3.17% | github.com/cockroachdb/cockroach/pkg/util/grpcutil.NewLocalRequestContext /go/src/github.com/cockroachdb/cockroach/pkg/util/grpcutil/grpc_util.go:39 655388 1.50% | github.com/cockroachdb/cockroach/pkg/sql.withStatement /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:3197 185693 0.42% | google.golang.org/grpc/metadata.NewOutgoingContext /go/src/github.com/cockroachdb/cockroach/vendor/google.golang.org/grpc/metadata/metadata.go:159 43812755 2.20% 2.20% 43812755 2.20% | context.WithValue /usr/local/go/src/context/context.go:533 ----------------------------------------------------------+------------- ```
|
This change is |
ajwerner
left a comment
There was a problem hiding this comment.
Nice find.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)
pkg/sql/catalog/descs/dist_sql_type_resolver.go, line 67 at r1 (raw file):
// NewDistSQLTypeResolver creates a new DistSQLTypeResolver. func NewDistSQLTypeResolver(descs *Collection, txn *kv.Txn) DistSQLTypeResolver { return DistSQLTypeResolver{
Should we just make this and the above NewTypeResolver method return a pointer? I feel like the whole path in NewTypeResolver where we return a zero value is a mistake because all the methods on that thing are going to panic.
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/descs/dist_sql_type_resolver.go, line 67 at r1 (raw file):
Previously, ajwerner wrote…
Should we just make this and the above
NewTypeResolvermethod return a pointer? I feel like the whole path inNewTypeResolverwhere we return a zero value is a mistake because all the methods on that thing are going to panic.
I was considering that, but noticed that vectorizedFlowCreator stores a descs.DistSQLTypeResolver by value, so it is a case where we are able to avoid a separate heap allocation by returning this by value and then storing it by value it in a larger struct. I'm mostly ambivalent though, so I'll defer to you.
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/descs/dist_sql_type_resolver.go, line 67 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I was considering that, but noticed that
vectorizedFlowCreatorstores adescs.DistSQLTypeResolverby value, so it is a case where we are able to avoid a separate heap allocation by returning this by value and then storing it by value it in a larger struct. I'm mostly ambivalent though, so I'll defer to you.
Thanks for checking. That's legit enough for me.
|
TFTR! bors r+ |
|
Build succeeded: |
This commit updates
DistSQLTypeResolverto implement all interfaces on apointer receiver instead of a value receiver. Implementing interfaces on values
is rarely the right choice, as it forces a heap allocation whenever the object
is boxed into an interface, instead of just forcing the pointer onto the heap
once (on its own or as part of a larger object) and then storing the pointer in
the interface header.
Before this commit, the use of value receivers was causing
HydrateTypeSlicetoallocate. Outside of #72798 and #72961, this was the single largest source of
heap allocations in TPC-E. With those two PRs applied,
HydrateTypeSlicewasaccounting for 2.30% of total heap allocations in the workload:
With this change, the heap allocation in
HydrateTypeSlicedisappears.With these three PRs combined, the largest source of heap allocations in the
workload is
context.WithValue, like the Go gods intended.