col/coldata: batch allocate memColumn objects #72961
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit updates `NewMemBatchWithCapacity` to batch allocate all necessary `memColumn` objects in a single chunk. Outside of cockroachdb#72798, this was the single largest source of heap allocations by object count (`alloc_objects`) in TPC-E. With cockroachdb#72798 applied, calls to `NewMemColumn` were responsible for **3.18%** of total heap allocations in the workload: ``` ----------------------------------------------------------+------------- flat flat% sum% cum cum% calls calls% + context ----------------------------------------------------------+------------- 355202097 97.92% | github.com/cockroachdb/cockroach/pkg/col/coldata.NewMemBatchWithCapacity /go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/batch.go:123 7561654 2.08% | github.com/cockroachdb/cockroach/pkg/sql/colmem.(*Allocator).NewMemColumn /go/src/github.com/cockroachdb/cockroach/pkg/sql/colmem/allocator.go:217 362763751 3.18% 3.18% 362763751 3.18% | github.com/cockroachdb/cockroach/pkg/col/coldata.NewMemColumn /go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/vec.go:202 ----------------------------------------------------------+------------- ``` Lower in the heap profile, we see that each of the other heap allocations that are performed once per call to `NewMemBatchWithCapacity` were observed 39625411 times. So we can estimate that the average batch contains `362763751 / 39625411 = 9.15` columns. This lines up with the improvement we see due to this change. Heap allocations due to `memColumn` objects drop by about a factor of 9, down to **0.33%** of all heap allocations: ``` ----------------------------------------------------------+------------- flat flat% sum% cum cum% calls calls% + context ----------------------------------------------------------+------------- 12082615 100% | github.com/cockroachdb/cockroach/pkg/sql/colmem.(*Allocator).NewMemBatchWithFixedCapacity /go/src/github.com/cockroachdb/cockroach/pkg/sql/colmem/allocator.go:131 12082615 0.33% 38.80% 12082615 0.33% | github.com/cockroachdb/cockroach/pkg/col/coldata.NewMemBatchWithCapacity /go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/batch.go:122 ----------------------------------------------------------+------------- ``` Despite this change, we will still probably want to address Jordan's TODO a few lines earlier about pooling all allocations it this level: https://github.com/cockroachdb/cockroach/blob/28bb1ea049da5bfb6e15a7003cd7b678cbc4b67f/pkg/col/coldata/batch.go#L113
|
This change is |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Nov 19, 2021
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 ----------------------------------------------------------+------------- ```
yuzefovich
approved these changes
Nov 19, 2021
There was a problem hiding this comment.
Nice!
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
|
TFTR! bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates
NewMemBatchWithCapacityto batch allocate all necessarymemColumnobjects in a single chunk. Outside of #72798, this was the singlelargest source of heap allocations by object count (
alloc_objects) in TPC-E.With #72798 applied, calls to
NewMemColumnwere responsible for 3.18% oftotal heap allocations in the workload:
Lower in the heap profile, we see that each of the other heap allocations that
are performed once per call to
NewMemBatchWithCapacitywere observed 39625411times. So we can estimate that the average batch contains
362763751 / 39625411 = 9.15columns.
This lines up with the improvement we see due to this change. Heap allocations
due to
memColumnobjects drop by about a factor of 9, down to 0.33% of allheap allocations:
Despite this change, we will still probably want to address Jordan's TODO a few
lines earlier about pooling all allocations it this level:
cockroach/pkg/col/coldata/batch.go
Line 113 in 28bb1ea