sql: pool some of the processor allocations #88973
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 change is |
This commit cleans up the `rowSourceToPlanNode` adapter (from the DistSQL processor to the planNode object) in the following manner: - it removes the incorrect call to `ConsumerClosed` of the wrapped input. This call was redundant (because the other side of the adapter `planNodeToRowSource` does the closure too) but was also incorrect since it could access the row source that was put back into the sync.Pool (and maybe even picked up by another query). See issue 88964 for more details. - it removes the checks around non-nil "metadata forwarder". These were needed when the local planNode and DistSQL processor engines were merged into one about four years ago, but nowadays the adapter always gets a non-nil forwarder. Release note: None
yuzefovich
force-pushed
the
fix-cleanup
branch
from
4af1b13 to
141c485
Compare
This commit makes it so that we now pool allocations of noop, planNodeToRowSource, and columnarizer processors. Additionally, trailing meta callbacks for these three as well as materializers are changed to not be anonymous functions to further reduce the allocations. Release note: None
yuzefovich
force-pushed
the
fix-cleanup
branch
from
141c485 to
0ff5952
Compare
|
Benchmark results are similar to what we had in #88608 before I had to pull out the last commit. |
|
The second commit here is an extension of what we had in #88608 (pooling of |
DrewKimball
approved these changes
Sep 30, 2022
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)
|
TFTR! bors r+ |
|
Build succeeded: |
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Oct 10, 2022
This commit removes a stale TODO about investigating why the materializers could not be released in all cases when they are created to wrap a row-by-row processor into the vectorized flow. The root cause was addressed in cockroachdb#88973 (the problem was that we could call `ConsumerClosed` on an already `Release`d object), so it is now safe to always release the materializers. For the same reason we no longer need to perform a deep copy of the closers when creating the materializer. Additionally, this commit removes a temporary allocation for a slice of releasables by directly modifying the main "tracking" slice. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Oct 10, 2022
89628: colexec: remove a couple of now-stale TODOs r=yuzefovich a=yuzefovich This commit removes a stale TODO about investigating why the materializers could not be released in all cases when they are created to wrap a row-by-row processor into the vectorized flow. The root cause was addressed in #88973 (the problem was that we could call `ConsumerClosed` on an already `Release`d object), so it is now safe to always release the materializers. For the same reason we no longer need to perform a deep copy of the closers when creating the materializer. Additionally, this commit removes a temporary allocation for a slice of releasables by directly modifying the main "tracking" slice. Release note: None Epic: None Co-authored-by: Yahor Yuzefovich <[email protected]>
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.
sql: clarify closing contract around plan node and row source adapters
This commit cleans up the
rowSourceToPlanNodeadapter (from theDistSQL processor to the planNode object) in the following manner:
ConsumerClosedof the wrappedinput. This call was redundant (because the other side of the adapter
planNodeToRowSourcedoes the closure too) but was also incorrect sinceit could access the row source that was put back into the sync.Pool (and
maybe even picked up by another query). See issue 88964 for more details.
needed when the local planNode and DistSQL processor engines were merged
into one about four years ago, but nowadays the adapter always gets
a non-nil forwarder.
Fixes: #88964.
Release note: None
sql: pool some of the processor allocations
This commit makes it so that we now pool allocations of noop,
planNodeToRowSource, and columnarizer processors. Additionally,
trailing meta callbacks for these three as well as materializers
are changed to not be anonymous functions to further reduce the
allocations.
Fixes: #88525.
Release note: None