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

More memory efficient query ledger-state command #4205

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Jul 20, 2022

This implements toEncoding of various ToJSON instances because it allows for more memory efficient lazy encoding.

As part of the change, the output of the query command will no longer be pretty printed.

Before uses about 16.4G:

$ time CARDANO_NODE_SOCKET_PATH=/Users/jky/wrk/iohk/mainnet/cardano-node.socket cardano-cli query ledger-state --mainnet > /tmp/ledger-state.bin

CARDANO_NODE_SOCKET_PATH=/Users/jky/wrk/iohk/mainnet/cardano-node.socket       302.63s user 14.17s system 91% cpu 5:47.89 total

After uses about 11.5G:

$ time CARDANO_NODE_SOCKET_PATH=/Users/jky/wrk/iohk/mainnet/cardano-node.socket $(./scripts/bin-path.sh cardano-cli) query ledger-state --mainnet > ledger-state.bin

CARDANO_NODE_SOCKET_PATH=/Users/jky/wrk/iohk/mainnet/cardano-node.socket       114.72s user 5.11s system 80% cpu 2:27.96 total

@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch from 6e165a9 to ab5b17a Compare July 20, 2022 15:44
@newhoggy newhoggy requested a review from lehins July 20, 2022 16:08
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch from ab5b17a to f37fc75 Compare July 20, 2022 16:22
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice work 👍 . You have introduce alignment where there wasn't any in the JSON instances. Lets stick to non-alignment. I think we should adopt an approach similar to ledger where we run a script with ormolu so there is no more ambiguity of the formatting.

cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
lehins
lehins previously requested changes Jul 20, 2022
Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

We should really try avoiding duplication. If at any point we need to refactor this code we'll have to do double amount of work. And even worse we have more room for mistakes, say if one of the duplicate implementations will get out of sync (eg. change one field name and then forget the other)

cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch 3 times, most recently from 691fb3b to 81ea950 Compare July 21, 2022 13:22
@newhoggy newhoggy dismissed stale reviews from lehins and Jimbo4350 July 21, 2022 13:27

comments addressed

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! Very minor syntactic adjustments suggested

cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Orphans.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch 2 times, most recently from 74e2d46 to ccbd58a Compare July 22, 2022 00:51
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch from ccbd58a to cbb8ea9 Compare July 26, 2022 13:07
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch 2 times, most recently from 4a35a0d to e411a98 Compare July 27, 2022 01:17
@newhoggy newhoggy requested a review from Jimbo4350 July 27, 2022 03:39
@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch from e411a98 to ff92c63 Compare July 27, 2022 09:59
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Good work! Please squash the commits.

@newhoggy newhoggy force-pushed the newhoggy/memory-efficient-query-ledger-state branch from ff92c63 to a79275b Compare July 28, 2022 02:18
@newhoggy
Copy link
Contributor Author

Squashed

@newhoggy
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 28, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 2552038 into master Jul 28, 2022
@iohk-bors iohk-bors bot deleted the newhoggy/memory-efficient-query-ledger-state branch July 28, 2022 07:07
@CarlosLopezDeLara CarlosLopezDeLara mentioned this pull request Aug 5, 2022
40 tasks
@CarlosLopezDeLara CarlosLopezDeLara mentioned this pull request Nov 3, 2022
19 tasks
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.

3 participants