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

Sacrifice build time for execution speed #640

Merged
merged 7 commits into from
Oct 24, 2022
Merged

Sacrifice build time for execution speed #640

merged 7 commits into from
Oct 24, 2022

Conversation

mike1729
Copy link
Member

@mike1729 mike1729 commented Oct 4, 2022

Description

Adds production profile to Cargo.toml that enables lto [1] and disables parallel compilation in LLVM [2]. Those changes trade compilation time for execution speed.

Bibliography

  1. https://doc.rust-lang.org/rustc/linker-plugin-lto.html
  2. https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have made neccessary updates to the Infrastructure
  • I have created new documentation

Copy link
Contributor

@Marcin-Radecki Marcin-Radecki left a comment

Choose a reason for hiding this comment

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

While enabling LTO is a project is generally non-controversial, as it is a mature enough feature that is used in LLVM for quite some time (not only by Rust, but also before 2015 by clang in C++), I think enabling that is fine in this PR. However, to achieve full effect of such change has to our system, more work is required:

  • most importantly, we need some figures before and after this change is introduced to compare what the speedup really is, as all those resources about LTO talk about theoretical speedup. Ideally, we should have some metrics that can measure that, e.g. CPU load would be the first one comes to my mind. Feature envs can help us measure so. If we don't rush with this PR, I'd say it's essential to be done before it's merged.
  • Having such a number then can be advertised on socials which makes PR effect, but we need to measure it first as point above states very carefully, it's easy to claim a lot of speedup on the first look
  • Then we should update this repo's README.md that we're using LTO, and docs for external validators that they should build their node, if there are any cargo build --release steps baked there

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
.github/workflows/nightly-pipeline.yaml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
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.

3 participants