Rewrite LedgerDB in-memory representation #1021
Merged
+724
−843
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
nfrisby
approved these changes
Sep 12, 2019
There was a problem hiding this comment.
Approved. I've made some comments along the lines of typos and e/amendments in comments, and some dead-"code" removal.
I didn't comb through the bulk of the new logic (in Ouroboros.Storage.LedgerDB.InMemory), since I'm unfamiliar there. However, the overview comments are good, the diff in the tests looks good, and Edsko said he'd already reviewed the changes with Lars.
ouroboros-consensus/test-storage/Test/Ouroboros/Storage/LedgerDB/OnDisk.hs
Show resolved
Hide resolved
ouroboros-consensus/test-storage/Test/Ouroboros/Storage/LedgerDB/InMemory.hs
Outdated
Show resolved
Hide resolved
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Sep 13, 2019
1021: Rewrite LedgerDB in-memory representation r=edsko a=edsko I rewrote the in-memory representation of the ledger DB. I just tested it against the Byron proxy running in profiling mode. With the old version of the Ledger DB, the top of the time profile looks like ``` ledgerDbReapply Ouroboros.Storage.LedgerDB.InMemory src/Ouroboros/Storage/LedgerDB/InMemory.hs:361:1-79 15.2 27.9 copyAndFreeze Data.ByteArray.Methods Data/ByteArray/Methods.hs:(237,1)-(240,21) 6.7 1.6 ledgerDbPush Ouroboros.Storage.LedgerDB.InMemory src/Ouroboros/Storage/LedgerDB/InMemory.hs:(332,1)-(356,11) 6.5 21.3 == Cardano.Chain.Common.KeyHash src/Cardano/Chain/Common/KeyHash.hs:31:16-17 4.3 7.0 ref Ouroboros.Storage.LedgerDB.InMemory src/Ouroboros/Storage/LedgerDB/InMemory.hs:(122,1)-(123,17) 4.2 0.0 ``` With the new version: ``` == Cardano.Chain.Common.KeyHash src/Cardano/Chain/Common/KeyHash.hs:31:16-17 29.5 47.3 copyAndFreeze Data.ByteArray.Methods Data/ByteArray/Methods.hs:(237,1)-(240,21) 3.3 1.9 compare Cardano.Chain.Common.KeyHash src/Cardano/Chain/Common/KeyHash.hs:32:16-18 2.9 6.6 lookup Data.Map.Internal Data/Map/Internal.hs:(561,1)-(567,18) 2.8 3.2 deserialiseIncremental Codec.CBOR.Read src/Codec/CBOR/Read.hs:(165,1)-(167,46) 2.8 1.3 ``` where the Ledger DB has disappeared _entirely_ from the profile. That top entry, a whopping 30% of time and 48% of memory allocation spent in `(==)` from `Cardano.Chain.Common.KeyHash` seems like it might benefit from a closer look also! The heap profile with the existing implementation:  And with the new implementation:  I updated the tests; the hardest part was modifying the tests to figure out which snapshots we store (that makes sense, that is precisely where the new implementation provides weaker guarantees than the old version), and then ran it over 1,000,000 tests, all of which passed. Co-authored-by: Edsko de Vries <[email protected]>
iohk-bors bot
added a commit
to input-output-hk/cardano-byron-proxy
that referenced
this pull request
Sep 13, 2019
43: Update for rewrite of Ledger DB r=avieth a=edsko This updates the Byron proxy for the rewrite of the Ledger DB in-memory representation (IntersectMBO/ouroboros-network#1021). Note however that the Byron proxy does not currently build against the latest `ouroboros-network` repo master: ``` src/exec/Shelley.hs:48:66: error: Not in scope: type constructor or class 'AnyResponderApp' | 48 | type ResponderVersions = Versions NodeToNodeVersion DictVersion (AnyResponderApp Peer NodeToNodeProtocols IO Lazy.ByteString) | ^^^^^^^^^^^^^^^ cabal: Failed to build exe:cardano-byron-proxy from cardano-byron-proxy-0.1.0.0. ``` Not sure where that is coming from. Draft PR because we should also update the hash (I tested with a local branch that rebases my Ledger DB patch against the version of `ouroboros-network` that the proxy currently builds against). Co-authored-by: Edsko de Vries <[email protected]> Co-authored-by: Alexander Vieth <[email protected]>
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.
I rewrote the in-memory representation of the ledger DB. I just tested it against the Byron proxy running in profiling mode. With the old version of the Ledger DB, the top of the time profile looks like
With the new version:
where the Ledger DB has disappeared entirely from the profile.
That top entry, a whopping 30% of time and 48% of memory allocation spent in
(==)fromCardano.Chain.Common.KeyHashseems like it might benefit from a closer look also!The heap profile with the existing implementation:
And with the new implementation:
I updated the tests; the hardest part was modifying the tests to figure out which snapshots we store (that makes sense, that is precisely where the new implementation provides weaker guarantees than the old version), and then ran it over 1,000,000 tests, all of which passed.