Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: pool some of the processor allocations #88608

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Sep 23, 2022

execinfra: fix temporary memory leak of expressions

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

execinfra: reuse output row allocation in the helper

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the pool-processors branch 2 times, most recently from bbdc020 to eaf86e0 Compare September 23, 2022 20:02
@yuzefovich yuzefovich added the do-not-merge bors won't merge a PR with this label. label Sep 23, 2022
@yuzefovich

This comment was marked as outdated.

@yuzefovich yuzefovich force-pushed the pool-processors branch 2 times, most recently from 322c829 to b07d9a3 Compare September 26, 2022 22:50
@yuzefovich yuzefovich removed the do-not-merge bors won't merge a PR with this label. label Sep 26, 2022
@yuzefovich yuzefovich force-pushed the pool-processors branch 3 times, most recently from 71f6f60 to d016fd2 Compare September 27, 2022 15:53
@yuzefovich yuzefovich marked this pull request as ready for review September 27, 2022 17:33
@yuzefovich yuzefovich requested a review from a team as a code owner September 27, 2022 17:33
@yuzefovich
Copy link
Member Author

First commit is #88831 and should be ignored here.

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice work!

Reviewed 10 of 10 files at r5, 1 of 1 files at r6, 2 of 2 files at r7.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @michae2, and @yuzefovich)


pkg/sql/colexec/columnarizer.go line 168 at r8 (raw file):

		execinfra.ProcStateOpts{
			// We append input to inputs to drain below in order to reuse the same
			// underlying slice from the pooled noopProcessor.

[nit] noopProcessor -> columnarizer

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @michae2)


pkg/sql/colexec/columnarizer.go line 168 at r8 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] noopProcessor -> columnarizer

Done.

@craig
Copy link
Contributor

craig bot commented Sep 27, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build failed:

@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build failed:

@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build failed:

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
@yuzefovich
Copy link
Member Author

Alright, these bors failures were legitimate, need to make some tweaks.

@yuzefovich
Copy link
Member Author

I'm still working through some things, so please hold off on reviewing - I'll ping once it's ready for a look.

@yuzefovich yuzefovich force-pushed the pool-processors branch 2 times, most recently from 01beaa7 to 1328362 Compare September 28, 2022 21:48
@yuzefovich
Copy link
Member Author

It's not as easy of a fix, so I removed the last commit that exposed a pre-existing bug (will file an issue soon) and will merge just the first two simple commits.

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build succeeded:

@craig craig bot merged commit e23f19c into cockroachdb:master Sep 28, 2022
@yuzefovich yuzefovich deleted the pool-processors branch September 28, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants