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

POC: Pooling allocator for head blocks for reduced memory fragmentation (experiment) #9777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

splitice
Copy link
Contributor

@splitice splitice commented Jun 23, 2023

What this PR does / why we need it:

We have observed particularly as the number of active streams increases Loki becomes more and more susceptable to memory fragmentation. Go is not a compacting GC so keeping around entire memory spans for one or two entries that are waiting on a chunk max time can be quite expensive.

This PR replaces individual strings with offset+length data referencing a chunk of memory (4096, or larger if line len exceeds this) thereby removing the strings being kept for block creation (these can be freed by GOGC). This PR adds new allocations on iteration and loading (e.g WAL) that otherwise would not exist. Thereby creating some GC pressure. This is because I have not replaced strings with []byte in loki, this would be a big PR.

  1. For example if you have a traffic pattern where you have:
  • 1,000 streams at low rate, the chunks for these are expected to chunk max time
  • 100 streams that are high rate, or peak to high rate

You can end up with spans (grouped by size) kept alive by entries from the 1,000 low rate strings

  1. With a bit of size variation you can end up with substantial go memory fragmentation by expanding the spans of different sizes

  2. If this patch was expanded to include a removal of string allocation the number of tracked objects by GOGC could be shrunk substantially.

In our observations this fragmentation leads to memory usage that is usually almost twice what it should be.

Another side effect of the current architecture is that it puts alot of stress on the GC for all the strings. This PR does not attempt to remove Loki's usage of string (e.g replace it with byte spans). That would be a PR in excess of this experiment.

Special notes for your reviewer:

Comments wanted.

To date with the Loki project PRs have gone to stale bot without any follow up. Prove Loki is not deaf to the community as it so often appears. Comment.

This is not intended to merge as-is, its intended to be a PoC.

If anyone is facing this today you can decrease the effect of this fragmentation by shrinking the block size, this casues less individual allocations of entries to be created. The major cause of fragmentation in Loki tends to be the strings for entries, by reducing the number of these in flight you in turn decrease Loki's susceptability to fragmentation.

You can also force ordered writes, this helps with both performance and memory fragmentation if your source allows for it.

Test Results

In testing (results taken after 6 hours of burn in)

Small Server: 76MB -> 32MB ram
Large Server: 3GB -> 2GB ram

Fragmentation is much reduced. Would be alot better I expect without any strings.

@splitice splitice requested a review from a team as a code owner June 23, 2023 04:54
@splitice splitice changed the title Pooling allocator for head blocks for reduced memory fragmentation (experiment) POC: Pooling allocator for head blocks for reduced memory fragmentation (experiment) Jun 23, 2023
@slim-bean
Copy link
Collaborator

To date with the Loki project PRs have gone to stale bot without any follow up. Prove Loki is not deaf to the community as it so often appears. Comment.

I know ☹️ , we are still a relatively small team and are trying to build and run the product for our SaaS product as well as maintain an open source project and I know we aren't doing great at supporting the community.

I'm sorry, I also know this isn't the answer most people want to hear. We do care about our community but we also do prioritize what generates revenue for us as that's what allows us to continue to build the product and make it available for free to everyone.

@splitice you have had some really thoughtful contributions in the past and we really do appreciate them, but I believe they often tend to go stale because they are complex and require thought/discussion which takes more time and folks become hesitant to engage at all.

Things are not as good as we want them to be and we are continuing to try to improve but it's a really hard problem, the team is very busy and a lot of the challenges we face running large scale infrastructure are not what a lot of our community members face and we are really trying to do our best to reconcile these differences.

But thank you for this PR, this is very thoughtful and you put a lot of really good work into it, as you did with all your previous PR's. We will do our best to engage and discuss as soon as we can!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants