Pool chunk slices instead of reusing them #6
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
Similar to #2, where buffers are reused instead of pooled. Reusing a buffer instead of pooling it means its capacity is never released. Therefore if a large spike in traffic causes that buffer to grow, the resources it holds onto are never returned even as the traffic decreases. This causes those large buffers to stick around and not get used. This change uses a `sync.Pool` instead of each `deltaSender` having its own buffer. This way buffers are automatically released when they aren't used.
ZoabKapoor
reviewed
Oct 30, 2024
|
No the 3GB screencap is from prod, under loads I just can't reproduce locally, I physically do not have the hardware for it haha. Every time I tried, it's shadowed by the actual ZK data and isn't significant enough to show up. |
bohhyang
reviewed
Oct 31, 2024
| // Return the queue that was too short to the pool | ||
| queuedUpdatesPool.Put(queue) | ||
| } | ||
| queue = new([]queuedResourceUpdate) |
There was a problem hiding this comment.
for my knowledge, this creates a new slice in the pool but what happens to the shorter one? Is it still in the pool or released? or just got extended longer?
There was a problem hiding this comment.
You can see just above, if the slice we get from the pool is too short, it returns it to the pool. I left a comment on what it means if ok is true
bohhyang
approved these changes
Oct 31, 2024
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.
Similar to #2, where buffers are reused instead of pooled. Reusing a buffer instead of pooling it means its capacity is never released. Therefore if a large spike in traffic causes that buffer to grow, the resources it holds onto are never returned even as the traffic decreases. This causes those large buffers to stick around and not get used.
This change uses a
sync.Poolinstead of eachdeltaSenderhaving its own buffer. This way buffers are automatically released when they aren't used.Here's a screenshot of the motivation behind this change, these unfreed buffers are responsible for about 20% of the memory usage of the observer:
