sql: remove a bunch of allocations from name and type resolution paths#57201
Conversation
|
This change is |
aec193b to
a344a0b
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
nit: not sure where "accessors" in the fifth commit's title comes from.
Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/execinfra/processorsbase.go, line 867 at r3 (raw file):
pb.SemaCtx.TypeResolver = resolver return pb.Out.Init(post, coreOutputTypes, &pb.SemaCtx, pb.EvalCtx, output)
I don't understand the third commit - where is the allocation removed from?
jordanlewis
left a comment
There was a problem hiding this comment.
Done.
Reviewable status:
pkg/sql/execinfra/processorsbase.go, line 867 at r3 (raw file):
Previously, yuzefovich wrote…
I don't understand the third commit - where is the allocation removed from?
The compiler detects that the SemaCtx argument to Init must be heap-allocated. Before, we constructed a new SemaCtx - seemingly on the stack. But because Init needed a heap-allocated SemaCtx, the compiler promotes the SemaCtx to the heap: the object escapes.
Now, we add space on ProcessorBase for a SemaCtx. At this point, ProcessorBase is already on the heap. So, the compiler can directly use the address of ProcessorBase.SemaCtx safely - no allocations needed.
Release note: None
This commit changes the VectorizedFlow to store its type resolver as a struct directly, rather than a pointer. This prevents a pointless allocation per flow. Release note: None
Previously, when initializing a processor, we'd need to allocate a fresh SemaCtx for every processor. Instead of doing this, we add a SemaCtx value inside of ProcessorBase to share the ProcessorBase-allocated memory. Release note: None
Release note: None
Previously, resolving a table would always need to heap allocate the TableName that is used to look up the names. This was unnecessary. Release note: None
a344a0b to
0bd8a5d
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r6, 4 of 4 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10.
Reviewable status:
pkg/sql/execinfra/processorsbase.go, line 867 at r3 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
The compiler detects that the
SemaCtxargument toInitmust be heap-allocated. Before, we constructed a newSemaCtx- seemingly on the stack. But becauseInitneeded a heap-allocatedSemaCtx, the compiler promotes theSemaCtxto the heap: the object escapes.Now, we add space on
ProcessorBasefor aSemaCtx. At this point,ProcessorBaseis already on the heap. So, the compiler can directly use the address ofProcessorBase.SemaCtxsafely - no allocations needed.
Got it, thanks.
jordanlewis
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
|
@jordanlewis @lucy-zhang I have a comment related to the "remove pointless log allocs on the hot path" commit. The very long log line changed by that commit adds the full descriptor information to recorded traces, which makes
|
|
Hmm... I'm thinking that we actually didn't mean to remove that from the trace. I think I should have used |
|
Hm, I made a couple of changes like that too, didn't realize |
|
Well, as I said, I think we should reconsider dumping that long info in each trace. Or at least split it into multiple logs. |
See individual commits. This PR shaves 3% of the total number of allocations in the FlowSetup benchmark via a variety of methods.