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

change(rust): Use link-time optimisation in release builds #4184

Merged
merged 4 commits into from
Apr 27, 2022
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 25, 2022

Motivation

As part of #4155, we want to speed up the zebrad initial sync.

Rust can optimise code at link-time, which reduces binary size, and seems to increase sync speed by about 5 minutes.

Specifications

https://doc.rust-lang.org/cargo/reference/profiles.html#lto

Solution

  • Turn on link-time optimisation for Rust code in release builds

Related cleanups:

This makes the zebrad binary about 50 MB smaller on my machine, and the sync about 5 minutes faster.

Review

Anyone can review this PR. It makes the sync faster, so it's a high priority.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-High 🔥 I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures lightwalletd any work associated with lightwalletd labels Apr 25, 2022
@teor2345 teor2345 self-assigned this Apr 25, 2022
@teor2345
Copy link
Contributor Author

I'm running a full sync test here:
https://github.com/ZcashFoundation/zebra/actions/runs/2218514256

This PR should be faster than the checkpoint PR.

@teor2345 teor2345 changed the base branch from main to update-checkpoints April 25, 2022 06:57
Base automatically changed from update-checkpoints to main April 25, 2022 14:43
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

This didn't seem to improve the sync time since opt-level=3 is already the default for release.

The other changes seem unrelated, did you mean to include them?

Cargo.toml Outdated Show resolved Hide resolved
@conradoplg
Copy link
Collaborator

The other changes seem unrelated, did you mean to include them?

Ah nevermind, this was caused by the automatic rebase

@teor2345
Copy link
Contributor Author

This PR should be faster than the checkpoint PR.

It's about 5 minutes faster.

@teor2345 teor2345 marked this pull request as ready for review April 25, 2022 20:08
@teor2345 teor2345 requested review from a team as code owners April 25, 2022 20:08
@teor2345 teor2345 requested review from conradoplg and removed request for a team April 25, 2022 20:08
@teor2345 teor2345 changed the title change(rust): Optimise all release builds, not just Docker builds change(rust): Use link-time optimisation in release builds Apr 25, 2022
@teor2345 teor2345 removed the request for review from a team April 25, 2022 20:46
@teor2345 teor2345 marked this pull request as draft April 25, 2022 20:47
@teor2345
Copy link
Contributor Author

I'm running a full sync test for LTO here:

We should merge if it is the same speed or faster than #4183, which was 5h53m.

@teor2345
Copy link
Contributor Author

I'm running a full sync test for LTO here:

* https://github.com/ZcashFoundation/zebra/actions/runs/2222788739

We should merge if it is the same speed or faster than #4183, which was 5h53m.

Failed due to #3991, restarted at:
https://github.com/ZcashFoundation/zebra/actions/runs/2222788739

@teor2345
Copy link
Contributor Author

We should merge if it is the same speed or faster than #4183, which was 5h53m.

Failed due to #3991, cherry-picked fix #4198, and restarted at:
https://github.com/ZcashFoundation/zebra/actions/runs/2225002822

@conradoplg
Copy link
Collaborator

conradoplg commented Apr 26, 2022

We should merge if it is the same speed or faster than #4183, which was 5h53m.

Failed due to #3991, cherry-picked fix #4198, and restarted at: https://github.com/ZcashFoundation/zebra/actions/runs/2225002822

Worked in 5h47m 🎉

but failed macos-latest test for some reason ("Error: The operation was canceled.", but it didn't timeout?). I'll retry it

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good, but I'm assuming the retry logic will be reviewed in #4198

@teor2345
Copy link
Contributor Author

macOS failed with:

Message: Opening metrics endpoint listener 127.0.0.1:55555 failed: Hyper(hyper::Error(Listen, Os { code: 48, kind: AddrInUse, message: "Address already in use" })). Hint: Check if another zebrad or zcashd process is running. Try changing the metrics endpoint_addr in the Zebra config.

https://github.com/ZcashFoundation/zebra/runs/6178143289?check_suite_focus=true#step:13:1439

Port 55555 was chosen randomly, but maybe it's in use on GitHub runners. (It seems suspiciously regular.)

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2022

update

✅ Branch has been successfully updated

@teor2345 teor2345 marked this pull request as ready for review April 26, 2022 23:15
@teor2345 teor2345 requested a review from conradoplg April 26, 2022 23:17
mergify bot added a commit that referenced this pull request Apr 27, 2022
@teor2345
Copy link
Contributor Author

Mergify queue PR #4184 failed with:

#27 exporting to image
#27 pushing layers 0.5s done
#27 ERROR: failed to authorize: failed to fetch oauth token: unexpected status: 401 Unauthorized
------
 > importing cache manifest from us-docker.pkg.dev/zealous-zebra/zebra/zebrad-test:4210-merge-buildcache:
------
------
 > exporting to image:
------
error: failed to solve: failed to fetch oauth token: unexpected status: 401 Unauthorized
Error: buildx failed with: error: failed to solve: failed to fetch oauth token: unexpected status: 401 Unauthorized

https://github.com/ZcashFoundation/zebra/runs/6185722377?check_suite_focus=true#step:9:2180

@teor2345
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2022

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 2423f59 into main Apr 27, 2022
@mergify mergify bot deleted the rust-optimise branch April 27, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants