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

Decouple benchmark dependencies from node #172

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Decouple benchmark dependencies from node #172

merged 1 commit into from
Jun 14, 2021

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jun 9, 2021

Taking aside the benchmark decoupling from the node crate, this PR also tries to remove unnecessary dependencies to speed-up builds and improve CI times.

Locally, cargo check went down from ~971 to ~948 crates.

native_executor_instance!(
pub Executor,
zeitgeist_runtime::api::dispatch,
zeitgeist_runtime::native_version,
Copy link
Contributor Author

@c410-f3r c410-f3r Jun 9, 2021

Choose a reason for hiding this comment

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

IIRC, frame_benchmarking::benchmarking::HostFunctions removal shouldn't be a problem. Still need to double check it

@c410-f3r c410-f3r requested review from sea212 and lsaether and removed request for sea212 June 9, 2021 14:45
@c410-f3r c410-f3r added the s:review-needed The pull request requires reviews label Jun 9, 2021
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Thanks, the computers love you for this!

Comment on lines -9 to +11
RUST_BACKTRACE=1 cargo fuzz run --fuzz-dir zrml/prediction-markets/fuzz full_workflow -- -runs=$RUNS
RUST_BACKTRACE=1 cargo fuzz run --fuzz-dir zrml/prediction-markets/fuzz pm_full_workflow -- -runs=$RUNS

RUST_BACKTRACE=1 cargo fuzz run --fuzz-dir zrml/swaps/fuzz full_workflow -- -runs=$RUNS
RUST_BACKTRACE=1 cargo fuzz run --fuzz-dir zrml/swaps/fuzz swaps_full_workflow -- -runs=$RUNS
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice fix you included along the way.

Comment on lines +16 to +20
if [[ "$package" == "zrml/market-commons" ]]; then
test_package_with_feature "$package" std
continue
fi

Copy link
Member

Choose a reason for hiding this comment

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

There might be multiple pallets in the future that don't have the runtime-benchmarks feature, but I think it is reasonable to fix it when the second pallet without that feature is included.

std = [
"frame-benchmarking?/std",
Copy link
Member

Choose a reason for hiding this comment

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

What does the ? operator do in this context? (I was not able to find it in the cargo book)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When activating an optional feature like serde/std, Cargo unconditionally also brings the whole dependency and this situation was causing some problems on CI, thus the reason for #157.

In this example, frame-benchmarking?/std means that the frame-benchmarking crate won't be included when calling cargo build --features std. rust-lang/cargo#8832

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, thanks for clarifying!

@lsaether lsaether added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jun 14, 2021
@lsaether lsaether merged commit 7ddc24a into main Jun 14, 2021
@c410-f3r c410-f3r deleted the caio-bench branch June 16, 2021 14:29
c410-f3r added a commit to c410-f3r/zeitgeist that referenced this pull request Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants