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

Reduce unnecessary dependencies #32

Closed
liamaharon opened this issue Aug 30, 2023 · 8 comments · Fixed by #86
Closed

Reduce unnecessary dependencies #32

liamaharon opened this issue Aug 30, 2023 · 8 comments · Fixed by #86
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@liamaharon
Copy link
Member

A significant amount of compilation time is spent on stuff like the kitchensink runtime, which clearly is unnecessary for try-runtime-cli.

Would be great if someone could

  1. Audit the compilation time and flag unnecessary dependencies that're dragged in
  2. Investigate if we can shuffle dependencies around and/or make changes upstream that would allow us to grab only what we need without unnecessary bloat
@liamaharon liamaharon added enhancement New feature or request good first issue Good for newcomers labels Aug 30, 2023
@eagr
Copy link
Contributor

eagr commented Dec 2, 2023

image

@liamaharon
Copy link
Member Author

We should be able to remove the kitchensink dep I think by creating a bare runtime only exposing the runtime apis we want to use.

@eagr
Copy link
Contributor

eagr commented Dec 3, 2023 •
edited 

cargo tree --invert --locked --package kitchensink-runtime

kitchensink-runtime v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
├── staging-node-cli v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│   └── substrate-cli-test-utils v0.1.0 (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│       [dev-dependencies]
│       └── try-runtime-core v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/core)
│           └── try-runtime-cli v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/cli)
└── staging-node-executor v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
    ├── staging-node-cli v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48) (*)
    └── try-runtime-cli v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/cli)
    [dev-dependencies]
    └── try-runtime-core v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/core) (*)

Yeah, we need to substitute substrate-cli-test-utils and staging-node-executor, to be specific.

This ain't trivial. The most viable path I could see is to identify and flag the optional dependencies upstream, and feature gate them all the way up.

@eagr
Copy link
Contributor

eagr commented Dec 3, 2023

And kitchensink-runtime may not even be the culprit.

cargo tree --invert --locked --package wasm-opt-sys

wasm-opt-sys v0.116.0
├── wasm-opt v0.116.0
│   └── substrate-wasm-builder v5.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│       [build-dependencies]
│       └── kitchensink-runtime v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│           ├── staging-node-cli v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│           │   └── substrate-cli-test-utils v0.1.0 (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│           │       [dev-dependencies]
│           │       └── try-runtime-core v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/core)
│           │           └── try-runtime-cli v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/cli)
│           └── staging-node-executor v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48)
│               ├── staging-node-cli v3.0.0-dev (https://github.com/paritytech/polkadot-sdk?branch=master#1d5d4a48) (*)
│               └── try-runtime-cli v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/cli)
│               [dev-dependencies]
│               └── try-runtime-core v0.5.1 (/Users/eagr/Projects/eagr/fork/substrate-try-runtime-cli/core) (*)
└── wasm-opt-cxx-sys v0.116.0
    └── wasm-opt v0.116.0 (*)

@liamaharon
Copy link
Member Author

liamaharon commented Dec 3, 2023 •
edited

This ain't trivial. The most viable path I could see is to identify and flag the optional dependencies upstream, and feature gate them all the way up.

I think there is a path to just removing the need for these deps entirely, rather than messing with their deps to make them lighter weight.

@eagr
Copy link
Contributor

eagr commented Dec 3, 2023

You mean to pull the necessary source code into this repo right? I'm worried that this might get out of hand in some way, but maybe not as bad as I think. Let me test it out.

@liamaharon
Copy link
Member Author

liamaharon commented Dec 3, 2023 •
edited

Not pull it out 1:1, but see what it is being used for, e.g. getting HostFunctions, and write our own minimal HostFunctions exposing only what we need instead of pulling in an entire runtime filled with crates just for the HostFunctions.

@eagr
Copy link
Contributor

eagr commented Dec 3, 2023

Tried substituting substrate-cli-test-utils. Giving up, too much to untangle in order to carve out kitchensink-runtime. It still seems to me that it's better to be fixed upstream by refactoring and feature-gating. Not entirely sure if this is related, I heard you're planning to replace wasmtime with polkavm. If that's the case, there would probably be a huge rewrite? I guess the problem should be flagged if not already.

And test code may be alright. It feels like it would become a maintenance burden (need to keep up with the sdk) or even backfire in the long run, going this route for production code.

@liamaharon liamaharon mentioned this issue May 7, 2024
Merged
liamaharon added a commit that referenced this issue May 26, 2024
@liamaharon @ggwpez
Inherents and empty block production overhaul (#86)
251438f 
A pre-requisite to producing empty blocks for MBMs is fixing inherents
for relay and parachains, and moving block production logic from being
tied to `fast_forward` to common logic.

It will also be very beneficial to allow producing blocks based on a
snapshot, without any live node (`--block-ws-uri`).

Based on the changes in this PR, I'll add MBM support to the
`on-runtime-upgrade` subcommand.

- Fixes inherent providers for relay and parachains
- Introduces a SMART inherent provider designed to work across different
chains, removing the requirement for a cli arg to specify which
inherents to use
- Moves inherent and block production to a common utils dir
- Removes requirement for a remote node to produce empty blocks
- Closes #32 (no longer compiles kitchensink runtime)

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Inherents and empty block production overhaul paritytech/try-runtime-cli
2 participants
@liamaharon @eagr