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

Make sure we never keep more than 1 ledger state in memory #639

Merged
merged 3 commits into from
Jun 11, 2021

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Jun 9, 2021

Ledger states have grown to take a lot of memory and we must be very careful with handling them. On rollbacks, db-sync parses a new ledger state from a file. We have to make sure that we don't keep a pointer to the old ledger state while parsing the new ledger state or this can cause big memory spikes.

Rollbacks also happen on startups. We have fixed startups in a slightly different way. Since the first message is always a MsgRollBackward we don't parse any ledger state before this message.

spike

After the fix the spike disappears:
no-spike

-- Ledger states are growing to become very big in memory.
-- Before parsing the new ledger state we need to make sure the old ledger state
-- is or can be garbage collected.
writeLedgerState env Nothing
mst <- findStateFromPoint env point delFiles
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we want to force a gc at this point, since the old ledger state is a very big memory chunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performMajorGC?

readStateUnsafe env = do
mState <- readTVar $ leStateVar env
case mState of
Nothing -> panic "ledger state is not found"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is still a work in progress, but panic message should have the module name in the message so if it ever gets hit we know where it came from.

mState <- readTVar $ leStateVar env
case mState of
Nothing -> panic "ledger state is not found"
Just st -> return st
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the code uses pure instead of return in preparation for https://gitlab.haskell.org/ghc/ghc/-/wikis/proposal/monad-of-no-return .

@@ -128,7 +127,7 @@ data LedgerEnv = LedgerEnv
{ leProtocolInfo :: !(Consensus.ProtocolInfo IO CardanoBlock)
, leDir :: !LedgerStateDir
, leNetwork :: !Ledger.Network
, leStateVar :: !(StrictTVar IO CardanoLedgerState)
, leStateVar :: !(StrictTVar IO (Maybe CardanoLedgerState))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could have an empty or minimal ledger state there instead of having the Maybe .

Copy link
Contributor Author

@kderme kderme Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leStateVar is read in 1 place: applyBlock. If we have a minimal state at this point it would simply lead to more cryptic errors.

We could try to make this type safe. This would probably need to follow the approach of ouroboros-network with typed protocols and parametrise LedgerEnv over the Nat n. Probably a pretty big refactoring.

leStateVar can be empty in 2 cases: after initiation and if loadLedgerAtPoint returns Left. In both cases we send to the node a FindIntersect message. The node replies with the point we should roll back and so loadLedgerAtPoint is called again.

@kderme kderme marked this pull request as ready for review June 10, 2021 13:21
@kderme kderme mentioned this pull request Jun 10, 2021
readStateUnsafe env = do
mState <- readTVar $ leStateVar env
case mState of
Nothing -> panic "LedgerState.readStateUnsafe: Ledger state is not found"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@erikd erikd merged commit 57daf09 into master Jun 11, 2021
@iohk-bors iohk-bors bot deleted the kderme/parse-ledger-once branch June 11, 2021 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants