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

Variable pipelining to optimize memory usage in the wallet restoration phase #3274

Merged

Conversation

paolino
Copy link
Collaborator

@paolino paolino commented May 12, 2022

  • I have implemented a pipelining number that depends on blockheight

Issue Number

ADP-1765

Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

Will look more tomorrow

lib/shelley/bench/latency-bench.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/PipeliningStrategy.hs Outdated Show resolved Hide resolved
lib/core/src/Ouroboros/Network/Client/Wallet.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/PipeliningStrategy.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/PipeliningStrategy.hs Outdated Show resolved Hide resolved
@paolino paolino changed the title variable pipelining Variable pipelining to optimize memory usage in the wallet restoration phase May 13, 2022
@paolino paolino force-pushed the paolino/ADP-1765/reduce-restoration-memory-footprint branch from 36ad806 to 002b2a4 Compare May 13, 2022 10:44
@paolino paolino self-assigned this May 13, 2022
@paolino paolino added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label May 13, 2022
@paolino paolino marked this pull request as ready for review May 16, 2022 06:41
@paolino paolino requested a review from Anviking May 16, 2022 06:55
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

I really like the PipeliningStrategy encapsulation 😊 — and recommend to focus the types on it. See comments.

lib/shelley/src/Cardano/Wallet/PipeliningStrategy.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/PipeliningStrategy.hs Outdated Show resolved Hide resolved
@paolino paolino force-pushed the paolino/ADP-1765/reduce-restoration-memory-footprint branch from cfcf20f to 66430c2 Compare May 17, 2022 08:56
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Punchier names! ✊

For example, I have no idea how to explain the meaning and purpose of the Pipelining type succinctly. But I can explain getPipelineSize: "getPipelineSize returns the size of the pipeline that should be used after encountering the given block (height)". Hence, I recommend to inline the type names.

lib/shelley/bench/restore-bench.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/Shelley/Network/Node.hs Outdated Show resolved Hide resolved
lib/core/src/Ouroboros/Network/Client/Wallet.hs Outdated Show resolved Hide resolved
@paolino paolino force-pushed the paolino/ADP-1765/reduce-restoration-memory-footprint branch from 581df10 to af6bdd8 Compare May 17, 2022 11:36
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

One last style issue to fix, then it's good to go! 😊

lib/core/src/Ouroboros/Network/Client/Wallet.hs Outdated Show resolved Hide resolved
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @paolino

There are a few sections of the code that are in excess of our line length limit. Would it be possible to amend those before merging?

Many thanks!

Jonathan

lib/shelley/bench/restore-bench.hs Outdated Show resolved Hide resolved
lib/shelley/bench/restore-bench.hs Outdated Show resolved Hide resolved
lib/shelley/bench/restore-bench.hs Outdated Show resolved Hide resolved
@paolino paolino force-pushed the paolino/ADP-1765/reduce-restoration-memory-footprint branch from d3e17b2 to 15c6ba0 Compare May 17, 2022 14:33
@paolino
Copy link
Collaborator Author

paolino commented May 18, 2022

bors r+

iohk-bors bot added a commit that referenced this pull request May 18, 2022
3274: Variable pipelining to optimize memory usage in the wallet restoration phase r=paolino a=paolino



- I have implemented a pipelining number that depends on blockheight

### Issue Number

ADP-1765


Co-authored-by: Paolo Veronelli <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 18, 2022

Build failed:

@paolino
Copy link
Collaborator Author

paolino commented May 18, 2022

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 18, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 4a62cef into master May 18, 2022
@iohk-bors iohk-bors bot deleted the paolino/ADP-1765/reduce-restoration-memory-footprint branch May 18, 2022 08:12
WilliamKingNoel-Bot pushed a commit that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants