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

Introduce lite-mode #444

Merged
merged 4 commits into from
May 3, 2024
Merged

Conversation

lordshashank
Copy link
Contributor

@lordshashank lordshashank commented Apr 25, 2024

Usage related changes

Would implement lite node that can be used with "--lite-node" tag which would skip blockhash calculation by replacing it with block_number hashes.

Development related changes

Benchmark Results

I performed benchmark testing using [mint_benchmark[(./scripts/benchmark/mint to compare the performance of the application in lite_mode and regular mode. Here are the results:

Mode Command Number of Mints Time (ms)
Lite Mode cargo run -- --lite-mode 200 11.172
Regular Mode cargo run 200 13.447
Lite Mode (Release) cargo run --release -- --lite-mode 1000 14.703
Regular Mode (Release) cargo run --release 1000 15.058

As expected, enabling lite_mode improves the performance of the application.

Checklist:

  • Checked the relevant parts of development docs
  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • Performed code self-review
  • Rebased to the latest commit of the target branch (or merged it into my branch)
  • Updated the docs if needed, including the TODO section
  • Linked the issues which this PR resolves
  • Updated the tests if needed; all passing (execution info)

@lordshashank
Copy link
Contributor Author

@ivpavici I have implemented lite mode by skipping blockhash calculation here and replacing it with hash of block_number.

Could you please see if the implementation is correct, I am not sure of few things like would calculating block_number before initializing header cause problem?, validity of initializing hash with from_prefixed_hex_string(block_number_hex) to reduce computation than generate_hex(), or if I have went completely wrong way here.

Also let me know if hash calculation has to be skipped somewhere else as well, thanks.

Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Have you done any benchmarking? What is the time impact of skipping block hash calculation?

@lordshashank
Copy link
Contributor Author

Not yet, I just implemented the functionality as per devnet-py, if its required let me know how I can do that?

@FabijanC
Copy link
Contributor

Not yet, I just implemented the functionality as per devnet-py, if its required let me know how I can do that?

Think of something, be creative :D

But one test could look like this, let's call the script mint_benchmark.sh:

#!/bin/bash

set -eu

N=$1

for _ in $(seq 1 $N); do
    curl -w "\n" -sSf localhost:5050/mint --json '{
        "amount": 1,
        "address": "0x1"
    }'
done

And you could run:

$ chmod +x mint_benchmark.sh
$ time ./mint_benchmark.sh 1000

Or replace 1000 with any number you wish to test. Of course, this assumes that somewhere on your computer you have Devnet running at port 5050. I'd recommend testing both cargo run and cargo run --release, both in lite and regular mode.

@ivpavici
Copy link
Contributor

yeah, please @lordshashank we would be very grateful for this test and report afterwards :)

@lordshashank
Copy link
Contributor Author

@FabijanC @ivpavici I have added benchmark report and test file as well, let me know if something else is needed.

@lordshashank lordshashank requested a review from FabijanC May 1, 2024 11:47
@FabijanC FabijanC mentioned this pull request May 2, 2024
10 tasks
@ivpavici
Copy link
Contributor

ivpavici commented May 2, 2024

thanks!
Seems like the speedup is negligible in --release mode... I will let @FabijanC decide about the merging. But either way this was helpful for us so you will definitely get a reward regardless if we merge or not!

Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Thanks for the table report in the PR description! How did you accumulate the times? I mean - are you reporting what you got on the first run? Or did you run it multiple times?

Also, I'm having doubts about DX and UX and if --lite-mode is the best approach. Perhaps we should have multiple CLI options like --block-hash <REAL|DUMMY>. We should consider what else can be skipped (other than block hash generation).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
crates/starknet-devnet/src/cli.rs Outdated Show resolved Hide resolved
scripts/benchmark/mint_benchmark.sh Outdated Show resolved Hide resolved
scripts/benchmark/mint_benchmark.sh Outdated Show resolved Hide resolved
crates/starknet-devnet-core/src/starknet/mod.rs Outdated Show resolved Hide resolved
crates/starknet-devnet-core/src/starknet/mod.rs Outdated Show resolved Hide resolved
@lordshashank
Copy link
Contributor Author

Times reported are of first run only.

For UX I suppose that best way could be to include --lite-mode which would include all the optimizations that we would be including in future and individual flags for each optimizations as well. But as currently there's just one, we can go with current flow ig, and change when we include new optimizations on future prs.

@FabijanC I have implemented the nits you suggested, let me know if anything else is to be done.

@lordshashank lordshashank requested a review from FabijanC May 2, 2024 17:39
Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Thanks!

@FabijanC FabijanC changed the title lite-mode initialized Introduce lite-mode May 3, 2024
@FabijanC FabijanC merged commit 3184b17 into 0xSpaceShard:main May 3, 2024
@lordshashank lordshashank deleted the lite-mode-impl branch May 3, 2024 11:47
@FabijanC
Copy link
Contributor

FabijanC commented May 3, 2024

@lordshashank Please, in the future, execute the tests to ensure the CI workflow passes. I will fix the failure coming from the merge of this PR.

@lordshashank
Copy link
Contributor Author

Really really sorry, I was thinking to do that after all the issues in pr were resolved, just forgot it at the end. Would surely keep in mind from next time.

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.

test the impact of "lite" mode on devnet performance and potentially introduce it
3 participants