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

Reduce memory usage with sharing #4826

Merged
merged 9 commits into from
Jan 30, 2025
Merged

Reduce memory usage with sharing #4826

merged 9 commits into from
Jan 30, 2025

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Jan 9, 2025

Description

Fixes #3486

It will probably be easier to review it by commit rather than PR as a whole. Especially for changes to the GOV rule

Memory benchmark

Here is run of a memory benchmark from the ledger-state package of loading a snapshot from preprod network

Before this PR:

Case                     Max          MaxOS           Live       Allocated    GCs
NewEpochState  1,136,825,184  2,834,300,928  1,136,825,184  24,981,680,224  5,816

With deserialization sharing implemented in this PR:

Case                   Max          MaxOS         Live       Allocated    GCs
NewEpochState  993,573,512  2,598,371,328  993,573,512  25,151,778,496  5,857

This benchamrk serves as confirmation that sharing works as expected during deserialization. Once we have integration ready for cardano-node-10.3 we'll be able to test sharing during chain replay as well.

Checklist

  • Commits in meaningful sequence and with useful messages
  • Tests added or updated when needed
  • CHANGELOG.md files updated for packages with externally visible changes

    New section is never added with the code changes. (See RELEASING.md)
  • Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
    versioning process.
  • Version bounds in .cabal files updated when necessary

    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Code formatted (use scripts/fourmolize.sh)
  • Cabal files formatted (use scripts/cabal-format.sh)
  • hie.yaml updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@lehins lehins force-pushed the lehins/reduce-memory-usage branch from c9f79f2 to 3b566f3 Compare January 10, 2025 06:24
@lehins lehins force-pushed the lehins/reduce-memory-usage branch 6 times, most recently from 79ce614 to 140b7ac Compare January 24, 2025 08:29
@lehins lehins marked this pull request as ready for review January 24, 2025 08:29
@lehins lehins requested a review from a team as a code owner January 24, 2025 08:29
@lehins lehins force-pushed the lehins/reduce-memory-usage branch from 140b7ac to 942d1bc Compare January 24, 2025 18:15
Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I'm a little confused by the choice of implementing decSharePlusCBOR vs decShareCBOR, but that's probably more because my unfamiliarity with this DecShareCBOR typeclass.

What I gather is something like this:

  • If you define getShare and decShareCBOR for a type, then if from "above" someone is calling decSharePlusCBOR for this type, then the getShare gets added to the shared state.
  • If you define decSharePlusCBOR instead, then it's up to you what you add to the shared state - but if you don't explicitly put anything, is there actually anything added to it?

Maybe we could discuss about this when we talk, no need to waste time writing!

@lehins lehins force-pushed the lehins/reduce-memory-usage branch from 942d1bc to 2ad0bf9 Compare January 30, 2025 01:38
@lehins lehins enabled auto-merge January 30, 2025 01:38
@lehins lehins merged commit bc10beb into master Jan 30, 2025
152 of 156 checks passed
@lehins lehins deleted the lehins/reduce-memory-usage branch January 30, 2025 04:01
@lehins lehins mentioned this pull request Jan 30, 2025
9 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.

Ensure Conway additions to ledger state use sharing
2 participants