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

feat: cache contracts before benching #254

Merged
merged 11 commits into from
Sep 27, 2024
Merged

feat: cache contracts before benching #254

merged 11 commits into from
Sep 27, 2024

Conversation

alexfertel
Copy link
Contributor

Use the cargo stylus cache to cache contracts in the CacheManager before running benches.

@alexfertel alexfertel added priority: 2 We will resolve this in a short timeframe. type: perf Performance work. effort: medium Default level of effort. labels Aug 22, 2024
@alexfertel alexfertel self-assigned this Aug 22, 2024
@netlify Netlify
Copy link

netlify bot commented Aug 22, 2024 •
edited
Loading

✅ Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 9256c4a
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/66f5b5db88e73f00087f7e29

bidzyyys
bidzyyys approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@bidzyyys bidzyyys 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

@alexfertel
Copy link
Contributor Author

Looks good

Doesn't work 😢

@qalisander qalisander changed the title feat: cache ERC-20 contract before benching feat: cache contracts before benching Sep 22, 2024
alexfertel
alexfertel commented Sep 23, 2024
View reviewed changes
Copy link
Contributor Author

@alexfertel alexfertel left a comment

Choose a reason for hiding this comment

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

It's great to see that this now works!

At first glance it seems like a 20k reduction in gas usage? Nice!

I can't approve cause I opened the PR lol

benches/src/access_control.rs Outdated
@@ -50,7 +50,9 @@ pub async fn bench() -> eyre::Result<Report> {
.wallet(EthereumWallet::from(bob.signer.clone()))
.on_http(bob.url().parse()?);

let contract_addr = deploy(&alice).await;
let contract_addr = deploy(&alice).await?;
crate::cache_contract(&alice, contract_addr)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this can be part of deploy? And maybe change deploy's signature to receive a enum CachePolicy { None, Bid(u64) } which determines whether we cache or not? (this may seem overengineered, but it is forward-compatible)

Copy link
Member

@qalisander qalisander Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also deploy_with_cache(&alice).await? could make, sense since 0 bid is always acceptable on nitro test node.

But I was thinking to have it separate for now. And later add analytics of how contracts perform with cache and without it in the single report. Also to track improvements of caching separate from contracts

benches/src/main.rs
Comment on lines +16 to 17
println!();
println!("{report}");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
println!();
println!("{report}");
println!("\n{report}");

Copy link
Member

Choose a reason for hiding this comment

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

It's kinda personal. For me add or remove another line is faster, then change a specific symbol:)

lib/e2e/src/account.rs Outdated Show resolved Hide resolved
scripts/bench.sh
@@ -5,9 +5,14 @@ MYDIR=$(realpath "$(dirname "$0")")
cd "$MYDIR"
cd ..

NIGHTLY_TOOLCHAIN=${NIGHTLY_TOOLCHAIN:-nightly}
NIGHTLY_TOOLCHAIN=${NIGHTLY_TOOLCHAIN:-nightly-2024-01-01}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe use a more recent nightly like 2024-09-01?

Copy link
Member

@qalisander qalisander Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alas, recent nightly versions do not work. That is another problem why we're pinned. Also I've checked that on stable not just binary size is bigger, but benchmarks perform %2-3 worse

Copy link
Member

@qalisander qalisander Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think in this pr we will update nightly till minimal working version everywhere in the project

scripts/bench.sh Outdated Show resolved Hide resolved
scripts/nitro-testnode.sh Outdated Show resolved Hide resolved
@alexfertel
Copy link
Contributor Author

I also think that panicking is fine here, since this is not user-facing, but not a strong opinion!

@qalisander
Copy link
Member

@alexfertel thanks for the review! It seems for me that caching doesn't make that big difference. But still

@ggonzalez94
Copy link
Collaborator

ggonzalez94 commented Sep 25, 2024 •
edited
Loading

@alexfertel this looks great! Thanks for continuing the work/review this.
One thing is that I believe we should have benchmarks for both cached and uncached runs of the contracts(ideally on a table that compares them side by side).

@qalisander WDYT?

@codecov Codecov
Copy link

codecov bot commented Sep 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.2%. Comparing base (d5ed376) to head (9256c4a).
Report is 5 commits behind head on main.

Additional details and impacted files

see 2 files with indirect coverage changes

@qalisander
Copy link
Member

qalisander commented Sep 25, 2024 •
edited
Loading

@ggonzalez94 yeah. I've been planning to do it. Check out gas benchmark report now

ggonzalez94
ggonzalez94 approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just a few nits and a questions. But looks ready to go 🚀

scripts/bench.sh Outdated Show resolved Hide resolved
benches/src/main.rs Outdated Show resolved Hide resolved
scripts/nitro-testnode.sh Outdated Show resolved Hide resolved
@qalisander
Copy link
Member

@ggonzalez94 @alexfertel It happened! Check out gas benchmarks now:) Just a small bug with pricing was fixed inside nitro node

@alexfertel
Copy link
Contributor Author

oooooh, they look much closer to the Solidity ones now! 🚀

Interesting that the merkle proof stayed the same. Overall looks great

@qalisander
Copy link
Member

Merkle proof doesn't write to storage. The bug was connected with charging fees multiple times for on-chain storage access

@qalisander qalisander merged commit 61000df into main Sep 27, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggonzalez94 ggonzalez94 ggonzalez94 approved these changes

@bidzyyys bidzyyys bidzyyys approved these changes

@qalisander qalisander Awaiting requested review from qalisander qalisander is a code owner

Assignees

@alexfertel alexfertel

Labels
effort: medium Default level of effort. priority: 2 We will resolve this in a short timeframe. type: perf Performance work.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexfertel @qalisander @ggonzalez94 @bidzyyys