chore(dataobj): reduce memory overhead of merging logs stripes #16833
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
Previously, pageReader instances would wait until the next call to pageReader.init to release any old resources, and even then it would only do it after creating a new reader (which means that the new reader wouldn't have access to the previous reader in the pool). This change allows pageReaders to proactively release Zstd decoders upon hitting EOF, and ensures that readers are closed before allocating new ones.
When calling basicReader.Reset and shrinking the number of columns, the old columns readers will be zeroed out and available for garbage collection. However, they are not closed, which means that they must wait until GC runs to release any resources like Zstd decoders. Closing them before clearing the remainder of the slice ensures that resources get released back to the pool and are available for reuse rather than just letting the GC clear them up.
Closing readers is required to guarantee that all resources are returned to pools, specifically for Zstd decoders. Without a Close call, some readers may be GC'd directly without having a chance to be reentered into the decoder pool.
While the loser tree guarantees that all sequences are closed if the entire input sequence is consumed, there are cases where the entire sequence _isn't_ consumed (like on encountering an error). For safety, we now close the loser tree on a defer to make sure that any resources associated with the tree are released.
Closing StreamsReader and LogsReaders helps release resources for Zstd decocders back into the pool.
This adds two metrics: * loki_dataobj_dataset_zstd_decoders_created_total * loki_dataobj_dataset_zstd_decoders_closed_total If zstdPool is being used properly, we would expect the rate of changes to loki_dataobj_dataset_zstd_decoders_created_total to be low (but not zero, due to pooled resources being garbage collected eventually). Similarly, if runtime.AddCleanup is being used properly, we would expect loki_dataobj_dataset_zstd_decoders_closed_total to be non-zero.
Update logs.Logs.flushSection to create the final section by doing several passes over the stripes, merging a group of stripes at a time until there is only one stripe remaining, which then becomes the section. This approach increases the flush time but lowers memory usage; as the loser tree merge requires opening one decompresser per column per stripe, the more stripes there are, the higher the memory usage of merging tables will be. The maximum number of stripes to merge at once can be configured with a new SectionStripeMergeLimit option. This value must be greater than 1.
rfratto
force-pushed
the
dataobj-zstd-decoder-improvements
branch
from
91ab242 to
f6ef9ae
Compare
rfratto
changed the title
github-actions
bot
added
the
type/docs
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.
Merging logs stripes was causing OOMs at scale. We found this to be for two main reasons:
Zstd decoders were not always being released back into the pool. Creating new decoders is expensive, so releasing them back into the pool will help relieve memory pressure.
Merging all stripes at once has a significant overhead.
This PR helps ensure that all Zstd decoders are released back into the pool and can be reused, and additionally adds a new config option to limit how many stripes are merged at once when forming a section, allowing operators to trade off between total flush time and total memory overhead.