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.

Sign up for GitHub

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

Jump to bottom

Sharing of values during deserialization #2557

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Sharing of values during deserialization #2557

merged 1 commit into from
Dec 1, 2021

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Nov 19, 2021 •
edited
Loading

This is an alternative approach to the one implemented in #2548

Applied sharing to all places possible in EpochState, including TxOuts in the UTxO. These are the results we get:

Case                                 Max          MaxOS           Live        Allocated      GCs
EpochState (FromCBOR)      1,199,503,544  3,557,818,368  1,199,503,544   70,260,877,216   63,501
EpochState (no-sharing)    1,970,142,176  4,795,138,048  1,910,834,120  408,093,302,920  388,393

@TimSheard
Copy link
Contributor

A bit of an overview might help. As I see it, we introduce a new class,
FromSharedCBOR t where ...
with an associated type family: Share t = ...
that specifies what is being shared when t is being de-serialized.
We need this flexibility, because different 't' may be sharing several different things. Usually (Share t) is a single type or a pair of types.
We also introduce some new monadic mechanisms, that hide (Share t) inside a state transformer. This introduces a lot of 'lifting" in the instances to move in and out of the monad.

The old PR #2548 dose similar things, but does them explicitly and manually. we trade lifting and monads, for pattern matching over tuples, and for explicit state transformers: fromCBORShare :: state -> (Decoder, state) in the few places it is needed.
My opinion is that the old PR is easier to understand and use.

@lehins lehins changed the title WIP: Sharing of values during deserialization Sharing of values during deserialization Nov 24, 2021
nc6
nc6 requested changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Made a bunch of comments, but otherwise I think this looks good!

eras/shelley/impl/src/Cardano/Ledger/Shelley/EpochBoundary.hs Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState.hs
decodeRecordNamed "DPState" (const 2) $ DPState <$> fromCBOR <*> fromCBOR
toCBOR DPState {_pstate, _dstate} =
encodeListLen 2
<> toCBOR _pstate -- We get better sharing when encoding pstate before dstate
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so we need to rebuild the ledger state. We should definitely coordinate the changes with the other things that update the ledger state serialisation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most definitely! We should talk about this, cause I am not sure what is the process when CBOR is changed in a backwards incompatible manner

eras/shelley/impl/src/Cardano/Ledger/Shelley/RewardUpdate.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/RewardUpdate.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/TxBody.hs Outdated Show resolved Hide resolved
libs/ledger-state/app/Main.hs
@@ -14,7 +14,10 @@ import System.IO
data Opts = Opts
{ -- | Path to the CBOR encoded NewEpochState data type, which will be used to
-- load into sqlite database
optsLedgerStateBinaryFile :: Maybe FilePath,
optsNewEpochStateBinaryFile :: Maybe FilePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean options in bench and app are the same? If that's the question than they look the same but they mean different things (eg. one is for reading, while another one is for writing). Moreover I suspect they will diverge in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant as with the other comment, sounds like it's addressed

libs/ledger-state/app/Main.hs
-- let dbFp = T.pack dbFpStr
-- km <- loadDbUTxO txIdSharingKeyMap dbFp
-- m <- loadDbUTxO noSharing dbFp
-- testKeyMap km m
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can still be useful. I will do a final cleanup of ledger-state package once we are done with memory optimizations

libs/ledger-state/bench/Memory.hs
optsNewEpochStateBinaryFile :: Maybe FilePath,
-- | Path to the CBOR encoded EpochState data type, which will be used to
-- load into sqlite database
optsEpochStateBinaryFile :: Maybe FilePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed comment

libs/ledger-state/bench/Memory.hs Show resolved Hide resolved
libs/small-steps/src/Data/Sharing.hs Outdated
-- =======================================

data Intern a = Intern
{ internMaybe :: a -> Maybe a,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment on this function? I guess it's the critical function here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed it could use a comment. I'll add one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some haddock, let me know if it makes sense and if it is enough of an explanation

nc6
nc6 reviewed Nov 30, 2021
View reviewed changes
libs/ledger-state/app/Main.hs Outdated Show resolved Hide resolved
nc6
nc6 approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Misc typos, otherwise looks good!

libs/small-steps/src/Data/Sharing.hs Outdated Show resolved Hide resolved
libs/small-steps/src/Data/Sharing.hs Outdated Show resolved Hide resolved
libs/small-steps/src/Data/Sharing.hs Outdated Show resolved Hide resolved
@JaredCorduan JaredCorduan mentioned this pull request Nov 30, 2021
Closed
@TimSheard @lehins
Sharing during CBOR deserialization:
71f6263 
* Added the Cardano.Ledger.Sharing module

* This supports sharing when deserializing.
  Made changes to share `Credential 'Staking crypto` and
  `KeyHash 'StakePool crypto` in `EpochState`

* Add shring to `NonMyopic`

* Benchmark EpochState sharing

* Avoid order of arguments with NamedFieldPuns in serialization

* Simplify `FromSharedCBOR` by removing `StateT` from `fromSharedCBOR`

* Apply sharing to `TxOut`
@lehins lehins merged commit 4e2d082 into master Dec 1, 2021
@iohk-bors iohk-bors bot deleted the lehins/sharing branch December 1, 2021 20:24
@lehins lehins mentioned this pull request Dec 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nc6 nc6 nc6 approved these changes

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@TimSheard TimSheard Awaiting requested review from TimSheard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lehins @TimSheard @nc6