-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-24.2: stats: use no-op DatumAlloc when decoding EncDatums #137061
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
blathers-crl
bot
force-pushed
the
blathers/backport-release-24.2-136780
branch
from
December 9, 2024 23:28
db8cd89
to
e919c08
Compare
blathers-crl
bot
added
the
blathers-backport
This is a backport that Blathers created automatically.
label
Dec 9, 2024
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
blathers-crl
bot
added
the
backport
Label PR's that are backports to older release branches
label
Dec 9, 2024
This commit makes so that `nil` `DatumAlloc` value makes the alloc struct a no-op, i.e. each datum type passed by value results in a separate allocation. This can be useful when the callers will keep only a subset of all allocated datums. Release note: None
This commit fixes a bounded memory leak that could previously occur due to usage of the DatumAlloc when decoding EncDatums into `tree.Datum`s during table stats collection. We have two use cases for decoded `tree.Datum`s: - we need it when adding _all_ rows into the sketch (we compute the datum's fingerprint ("hash") to use in the distinct count estimation). This usage is very brief and we don't need the decoded datum afterwards. - we also need it when sampling _some_ rows when we decide to keep a particular row. In this case, the datum is needed throughout the whole stats collection job (or until it's replaced by another sample) for the histogram bucket computation. The main observation is that the DatumAlloc allocates datums in batches of 16 objects, and even if at least one of the objects is kept by the sample, then all others from the same batch are as well. We only perform memory accounting for the ones we explicitly keep, yet others would be considered live by the Go runtime, resulting in a bounded memory leak. This behavior has been present since forever, but it became more problematic in 24.2 release with the introduction of dynamically computing the sample size. To go around this problematic behavior this commit uses `nil` DatumAlloc which makes it so that every decoded datum incurs a separate allocation. This will have a minor performance hit and increase in total number of allocations, but at least most of them should be short-lived. Alternatively, we could use an "operating" DatumAlloc during fingerprinting and a no-op during the sampling, but we'd need to explicitly nil out the decoded datum after fingerprinting which would result in decoding some datums twice. Release note: None
yuzefovich
force-pushed
the
blathers/backport-release-24.2-136780
branch
from
December 10, 2024 21:04
e919c08
to
940ac56
Compare
mgartner
approved these changes
Dec 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
backport
Label PR's that are backports to older release branches
blathers-backport
This is a backport that Blathers created automatically.
O-robot
Originated from a bot.
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 #136780 on behalf of @yuzefovich.
/cc @cockroachdb/release
tree: make nil DatumAlloc act as a no-op
This commit makes so that
nil
DatumAlloc
value makes the alloc struct a no-op, i.e. each datum type passed by value results in a separate allocation. This can be useful when the callers will keep only a subset of all allocated datums.Release note: None
stats: use no-op DatumAlloc when decoding EncDatums
This commit fixes a bounded memory leak that could previously occur due to usage of the DatumAlloc when decoding EncDatums into
tree.Datum
s during table stats collection. We have two use cases for decodedtree.Datum
s:The main observation is that the DatumAlloc allocates datums in batches of 16 objects, and even if at least one of the objects is kept by the sample, then all others from the same batch are as well. We only perform memory accounting for the ones we explicitly keep, yet others would be considered live by the Go runtime, resulting in a bounded memory leak. This behavior has been present since forever, but it became more problematic in 24.2 release with the introduction of dynamically computing the sample size. To go around this problematic behavior this commit uses
nil
DatumAlloc which makes it so that every decoded datum incurs a separate allocation. This will have a minor performance hit and increase in total number of allocations, but at least most of them should be short-lived. Alternatively, we could use an "operating" DatumAlloc during fingerprinting and a no-op during the sampling, but we'd need to explicitly nil out the decoded datum after fingerprinting which would result in decoding some datums twice.Fixes: #136394.
Release note: None
Release justification: bug fix.