fix(ledger,shred-network): memory leak #510
Open
+314
−203
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
dnut
force-pushed
the
dnut/fix/shred-network/keepup
branch
2 times, most recently
from
c258bfa to
20934fe
Compare
this is a first step towards shreds themselves being immutable. the mutations now act on a mutable shred slice instead of the shred itself. but the shred still needs to be mutated sometimes so payloadMut serves this purpose until mutations of Shred is eliminated
previously any shreds created in shred processor or insertShreds leaked. this establishes clarified ShredInserter.insertShreds lifetime rules: - shreds passed in are owned by caller and must be freed by caller (applied this in shred processor) - shreds created in the function need to be cleaned up in the function, unless... - shreds returned from the function need to be owned by the caller, so they need a sufficient lifetime that exceeds the state of insertshreds and the shreds that were passed in. this is currently solved by duping the shreds.
…f 16 in my test i saw its memory usage go down from 1 GB to 6 MB which is obviously a much bigger drop than 16x, so I'm not sure how easy it is to isolate the impact of this change. Nonetheless I don't think this cache needs to go for 128 slots since typically repair does not need to go back more than a few slots. there is some potential to optimize this for the startup process of catching up from behind. but it's working as is in my testing
dnut
force-pushed
the
dnut/fix/shred-network/leak
branch
from
7185a91 to
f2a99d8
Compare
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.
This PR is stacked on top of #493 and needs to merge after that.
Leaks fixed in this pr
Shred lifetime clarification
Previously there were a bunch of TODOs in the code about figuring out the lifetime of data used during insertShreds. This led to the memory leak and would also have lead to memory errors once the ShredInserter was hooked up to other validator components that read its data. I cleaned this up a bit by establishing and implementing some basic guidelines for shred lifetimes.
To satisfy points 2 and 3, it became necessary to clone any shreds that are returned by insertShreds. I explored some alternatives to avoid this cloning, such as reference counting. As a first step, I tried to make Shred immutable. So you'll see some changes in this PR that are steps in the direction of Shred being immutable. But this turned into a very deep rabbit hole that wasted about a day of my time, so I decided it was out of scope and went with the basic cloning approach for this PR. I split off most of the WIP immutable-Shred code into a separate branch here: dnut/refactor/ledger/immutable-shred