This repository has been archived by the owner on Sep 28, 2022. It is now read-only.
Don't use batch size for channel buffers #258
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
Channel buffers are there to let workers do a bit more work while things are running, but batch size is for the amount of stuff we want to accumulate before dropping a whole batch of computed results off with the server. There's no reason for the channel buffer to be that size, and having it be that size (which is typically at least 65536) means that each import worker is typically sitting on a channel that has about a million interface-flavored objects containing pointers to records in it, and suddenly we have memory pressure.
In a previous version of this, I thoughtfully resliced slices to [:0] rather than allocating. That's a great idea, except that you don't necessarily ever see a given shard again. If you don't see that shard again, or if you don't get as many items in it, all the old record pointers are still sitting there in reachable parts of the slice's backing store, preventing them from ever being garbage collected.
|
FWIW: a workload that was getting OOM-killed somewhere past 30GB of memory before this is staying under 1GB of memory after this. This is the mysterious memory leak I was pretty sure I'd detected in imagine, but hadn't fixed yet. |
|
I saw this in uber's go style guide, https://github.com/uber-go/guide/blob/master/style.md#channel-size-is-one-or-none. |
|
There's some utility to small-positive channel sizes, although mostly as an implementation of semaphores, but they can be useful for buffering to provide some insulation against bursty things. But we definitely don't need 65k+ sized channels. |
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Channel buffers are there to let workers do a bit more work
while things are running, but batch size is for the amount of stuff
we want to accumulate before dropping a whole batch of computed
results off with the server. There's no reason for the channel buffer
to be that size, and having it be that size (which is typically at
least 65536) means that each import worker is typically sitting on
a channel that has about a million interface-flavored objects containing
pointers to records in it, and suddenly we have memory pressure.
(Fixes, I hope, #4.)