release-22.2: sql: pool some of the processor allocations #88976
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
![blathers-crl[bot]](https://avatars.githubusercontent.com/in/59700?s=60&v=4){kind=small}
blathers-crl
bot
force-pushed
the
blathers/backport-release-22.2-88608
branch
from
dbab3ca to
36b1c8b
Compare
|
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
blathers-crl
bot
added
blathers-backport
|
This change is |
|
Hold off on reviewing - this should go in together with #88973 which I'm still working on. We might target 22.2.1 for this. |
This commit fixes the way we reuse the ProcOutputHelpers. Previously, we would not perform the deep reset of the render expressions which could lead to those expressions being live for longer than needed (namely till the helper is reused the next time). This matters when the ProcessorBase is reused which currently only happens when TableReaders are used from the sync.Pool. This probably was a pretty bad issue before the vectorized engine, but nowadays its impact seems minor. Still, it's good to fix it, especially given the following commit which will introduce more pooling of processors. Release note: None
This commit makes it so that we reuse the output row in the ProcOutputHelper if possible. Additionally, it removes a row alloc object since it is not helpful and only leeds to wasteful allocations. It also removes a redundant integer for the number of "internal columns". Release note: None
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
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
blathers/backport-release-22.2-88608
branch
from
36b1c8b to
fb6d237
Compare
|
I included #88973, but this should wait for 22.2.1 since the impact is not that significant. |
yuzefovich
added
do-not-merge
|
@DrewKimball PTAL |
DrewKimball
approved these changes
Oct 5, 2022
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
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.
Backport 2/2 commits from #88608 on behalf of @yuzefovich.
Backport 2/2 commits from #88973 on behalf of @yuzefovich.
/cc @cockroachdb/release
execinfra: fix temporary memory leak of expressions
execinfra: reuse output row allocation in the helper
sql: clarify closing contract around plan node and row source adapters
sql: pool some of the processor allocations
Release justification: low risk perf regression fix.