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

CSUB-1231: Change Benchmarking script and Release builds to use --profile=production #435

Closed
wants to merge 3 commits into from

Conversation

BradleyOlson64
Copy link
Contributor

Enacting the same changes in CC3 as were made in CCNext here https://github.com/gluwa/creditcoin3-next/pull/137

As described here using --release to build for benchmarking and for live network images/artifacts results in network performance loss due to missing optimizations.

Anecdotally I've observed that execution time weights are about 10% worse using --release vs --profile=production

This pr doesn't try to change all CI jobs to use --profile=production though, because compiling that way takes about 2x longer. We don't want to waste loads of money on unnecessary compilation work.

@BradleyOlson64 BradleyOlson64 requested a review from a team as a code owner August 5, 2024 19:33
@BradleyOlson64 BradleyOlson64 removed the request for review from a team August 5, 2024 19:33
Copy link

github-actions bot commented Aug 5, 2024

For full LLVM coverage report click here!

rustlang-dev
rustlang-dev previously approved these changes Aug 5, 2024
DylanVerstraete
DylanVerstraete previously approved these changes Aug 6, 2024
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Minor change requested.

@@ -111,22 +111,22 @@ jobs:
uses: gluwa/cargo@dev
with:
command: build
args: --release ${{ needs.setup.outputs.build_options}}
args: --profile=production ${{ needs.setup.outputs.build_options}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same should be applied in wasm.yml as well, otherwise looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applying this change in wasm.yml created a rather confusing wasm build error in CI.

I'm not so sure this change is helpful, since release.yml appears to run its own independent wasm build. We can do without this right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do without this right?

IDK tbh. For Testnet & Mainnet releases we typically use the wasm blob from the release, e.g. the once created in release.yml. For automated Devnet upgrades we use the one from wasm.yaml. Both are compiled in the same way minus a few feature flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am reading the error logs at https://github.com/gluwa/creditcoin3/actions/runs/10288255898/job/28473252780 correctly it is a problem with how the profile information is specified, which I would think should be the same between the 2 yaml files but for some reason it isn't.

Let's look into it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR: experimenting in #437. Let's not make any more changes here until we know what's going on.

@BradleyOlson64
Copy link
Contributor Author

Closing this PR, as the team decided to include these changes upstream in CCNext and let them trickle down.

@BradleyOlson64 BradleyOlson64 deleted the brad-change-build-profile branch August 8, 2024 22:32
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.

4 participants