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

Contracts: Fix terminate benchmark #3558

Merged

Conversation

pgherveou
Copy link
Contributor

No description provided.

@pgherveou pgherveou requested a review from athei as a code owner March 4, 2024 10:50
@pgherveou pgherveou added the R0-silent Changes should not be mentioned in any release notes label Mar 4, 2024
Copy link
Contributor Author

@pgherveou pgherveou Mar 4, 2024

Choose a reason for hiding this comment

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

We could also introduce a seal_terminate_per_locked_dependency but the extra PoV should be pretty low and will be refunded anyway with PoV Reclaim

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the benchmarks to see how bad it is. I don't think the reclaim is working, yet. The linked PR is only the node side. However, terminate is rare enough so that I wouldn't optimize too much for it so we are probably good.

@pgherveou pgherveou changed the title Contracts: Fix delegate_dependency length in benchmark Contracts: Fix terminate benchmark Mar 4, 2024
@pgherveou pgherveou enabled auto-merge March 4, 2024 14:22
substrate/frame/contracts/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the benchmarks to see how bad it is. I don't think the reclaim is working, yet. The linked PR is only the node side. However, terminate is rare enough so that I wouldn't optimize too much for it so we are probably good.

pgherveou and others added 2 commits March 5, 2024 22:28
@pgherveou
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_contracts

@command-bot
Copy link

command-bot bot commented Mar 5, 2024

@pgherveou https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5445920 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 5-b08d8bbb-009c-4b41-8094-21932a8ead5e to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 5, 2024

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5445920 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5445920/artifacts/download.

@pgherveou
Copy link
Contributor Author

Weights are much higher (194%) as expected

Since this is a rare operation and PoV refund is around the corner (runtime PR merged here do you think it's still worth adding a terminate_per_locked_delegate_dependency?

@athei
Copy link
Member

athei commented Mar 6, 2024

I think then it is fine to just use the worst case here.

@pgherveou pgherveou added this pull request to the merge queue Mar 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 6, 2024
@athei
Copy link
Member

athei commented Mar 6, 2024

Weird. Those should not have been affected by your changes here?

@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 7, 2024

Weird. Those should not have been affected by your changes here?

oups:sorry this is a comment for #3585

@pgherveou pgherveou enabled auto-merge March 8, 2024 17:35
@pgherveou pgherveou added this pull request to the merge queue Mar 8, 2024
Merged via the queue into master with commit 1fe3c5f Mar 8, 2024
129 of 130 checks passed
@pgherveou pgherveou deleted the pg/contracts-fix-delegate_dependency-length-in-benchmark branch March 8, 2024 18:48
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Co-authored-by: Alexander Theißen <[email protected]>
Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants