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

Removed extra messages benchmarks #1279

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Conversation

svyatonik
Copy link
Contributor

rationale: #1268

I was trying to keep thee benchmarks, but this is hard to do, since they're declared within benchmarks_instance_pallet! {} macro itself && this macro has its own requirements (e.g. you can't hide the whole benchmark behind the feature flag, you can't hide the whole body behind feature flag, because first line must be let i in ... and so on). So let's remove it for now.

Copy link
Collaborator

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Code changes look good, but what's the plan for having benchmarks here in the future? Should we open an issue detailing what's needed?

@svyatonik
Copy link
Contributor Author

what's the plan for having benchmarks here in the future? Should we open an issue detailing what's needed?

Let's just drop them for now. They were helpful when I've been working on weight formulae. Now they only serve as a proof of some assumptions I have made and these assumptions are only checked manually. Since this is very specific case imo (most of pallets don't need such extra benchmarks - see our other pallets for example) I don't think it worth to waste resources on changing benchmarks macro or on maintaining this code. We may reconsider this later ofc - there's always git history :)

@svyatonik svyatonik merged commit e98d682 into master Jan 11, 2022
@svyatonik svyatonik deleted the remove-extra-messages-benchmarks branch January 11, 2022 13:00
svyatonik pushed a commit that referenced this pull request Jul 17, 2023
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants