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

[Merged by Bors] - Improve compile time #1989

Closed
wants to merge 8 commits into from
Closed

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Nov 27, 2020

Issue Addressed

Closes #1264

Proposed Changes

  • Milagro BLS: tweak the feature flags so that Milagro doesn't get compiled if we're using BLST. Profiling showed that it was consuming about 1 minute of CPU time out of 60 minutes of CPU time (real time ~15 mins). A 1.6% saving.
  • Reduce monomorphization: compiling for 3 different EthSpec types causes a heck of a lot of generic functions to be instantiated (monomorphized). Removing 2 of 3 cuts the LLVM+linking step from around 250 seconds to 180 seconds, a saving of 70 seconds (real time!). This applies only to make and not the CI build, because we test with the minimal spec on CI.
  • Update web3 crate to v0.13. This is perhaps the most controversial change, because it requires axing some deposit contract tools from lcli. I suspect these tools weren't used much anyway, and could be maintained separately, but I'm also happy to revert this change. However, it does save us a lot of compile time. With [Merged by Bors] - Upgrade to tokio 0.3 #1839, we now have 3 versions of Tokio (and all of Tokio's deps). This change brings us down to 2 versions, but 1 should be achievable once web3 (and reqwest) move to Tokio 0.3.
  • Remove lcli from the Docker image. It's a dev tool and can be built from the repo if required.

@michaelsproul michaelsproul added the work-in-progress PR is a work-in-progress label Nov 27, 2020
@michaelsproul michaelsproul changed the base branch from stable to unstable November 27, 2020 13:04
@michaelsproul michaelsproul force-pushed the compile-time branch 2 times, most recently from e0b7567 to 38aa24d Compare November 27, 2020 23:21
@michaelsproul michaelsproul added ready-for-review The code is ready for review infra-ci and removed work-in-progress PR is a work-in-progress labels Dec 7, 2020
@michaelsproul michaelsproul marked this pull request as ready for review December 7, 2020 23:26
@michaelsproul michaelsproul added the low-hanging-fruit Easy to resolve, get it before someone else does! label Dec 7, 2020
@michaelsproul
Copy link
Member Author

Test failure is just that racy DHT test.

This is ready for review

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Nice, I'm happy to lose those lcli commands. I'm pretty sure I'm the only one that uses/used them.

I'm sure this will collectively save a lot of time for devs! :D

@michaelsproul
Copy link
Member Author

Yay!

bors r+

@bors
Copy link

bors bot commented Dec 8, 2020

Merge conflict.

@michaelsproul
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Dec 8, 2020
## Issue Addressed

Closes #1264

## Proposed Changes

* Milagro BLS: tweak the feature flags so that Milagro doesn't get compiled if we're using BLST. Profiling showed that it was consuming about 1 minute of CPU time out of 60 minutes of CPU time (real time ~15 mins). A 1.6% saving.
* Reduce monomorphization: compiling for 3 different `EthSpec` types causes a heck of a lot of generic functions to be instantiated (monomorphized). Removing 2 of 3 cuts the LLVM+linking step from around 250 seconds to 180 seconds, a saving of 70 seconds (real time!). This applies only to `make` and not the CI build, because we test with the minimal spec on CI.
* Update `web3` crate to v0.13. This is perhaps the most controversial change, because it requires axing some deposit contract tools from `lcli`. I suspect these tools weren't used much anyway, and could be maintained separately, but I'm also happy to revert this change. However, it does save us a lot of compile time. With #1839, we now have 3 versions of Tokio (and all of Tokio's deps). This change brings us down to 2 versions, but 1 should be achievable once web3 (and reqwest) move to Tokio 0.3.
* Remove `lcli` from the Docker image. It's a dev tool and can be built from the repo if required.
@bors
Copy link

bors bot commented Dec 8, 2020

Timed out.

@AgeManning
Copy link
Member

racey DHT should be fixed in current unstable, is this not up to date? 🤔

@michaelsproul
Copy link
Member Author

It wasn't up to date when the DHT test failed, but it is now. Looks like Bors failed because of some issue with Ganache in the eth1 simulator: https://github.com/sigp/lighthouse/runs/1515897230?check_suite_focus=true

bors retry

bors bot pushed a commit that referenced this pull request Dec 8, 2020
## Issue Addressed

Closes #1264

## Proposed Changes

* Milagro BLS: tweak the feature flags so that Milagro doesn't get compiled if we're using BLST. Profiling showed that it was consuming about 1 minute of CPU time out of 60 minutes of CPU time (real time ~15 mins). A 1.6% saving.
* Reduce monomorphization: compiling for 3 different `EthSpec` types causes a heck of a lot of generic functions to be instantiated (monomorphized). Removing 2 of 3 cuts the LLVM+linking step from around 250 seconds to 180 seconds, a saving of 70 seconds (real time!). This applies only to `make` and not the CI build, because we test with the minimal spec on CI.
* Update `web3` crate to v0.13. This is perhaps the most controversial change, because it requires axing some deposit contract tools from `lcli`. I suspect these tools weren't used much anyway, and could be maintained separately, but I'm also happy to revert this change. However, it does save us a lot of compile time. With #1839, we now have 3 versions of Tokio (and all of Tokio's deps). This change brings us down to 2 versions, but 1 should be achievable once web3 (and reqwest) move to Tokio 0.3.
* Remove `lcli` from the Docker image. It's a dev tool and can be built from the repo if required.
@bors
Copy link

bors bot commented Dec 8, 2020

Build failed:

  • eth1-simulator-ubuntu

@michaelsproul
Copy link
Member Author

Grr, that eth1 simulator. I'm going to see if I've somehow broken it on this branch by testing locally

@michaelsproul
Copy link
Member Author

Looks like I was overeager in removing compat() shims. They're also required for Tokio 0.2. Think I've got them all now. Will r+ optimistically:

bors retry

bors bot pushed a commit that referenced this pull request Dec 9, 2020
## Issue Addressed

Closes #1264

## Proposed Changes

* Milagro BLS: tweak the feature flags so that Milagro doesn't get compiled if we're using BLST. Profiling showed that it was consuming about 1 minute of CPU time out of 60 minutes of CPU time (real time ~15 mins). A 1.6% saving.
* Reduce monomorphization: compiling for 3 different `EthSpec` types causes a heck of a lot of generic functions to be instantiated (monomorphized). Removing 2 of 3 cuts the LLVM+linking step from around 250 seconds to 180 seconds, a saving of 70 seconds (real time!). This applies only to `make` and not the CI build, because we test with the minimal spec on CI.
* Update `web3` crate to v0.13. This is perhaps the most controversial change, because it requires axing some deposit contract tools from `lcli`. I suspect these tools weren't used much anyway, and could be maintained separately, but I'm also happy to revert this change. However, it does save us a lot of compile time. With #1839, we now have 3 versions of Tokio (and all of Tokio's deps). This change brings us down to 2 versions, but 1 should be achievable once web3 (and reqwest) move to Tokio 0.3.
* Remove `lcli` from the Docker image. It's a dev tool and can be built from the repo if required.
@bors bors bot changed the title Improve compile time [Merged by Bors] - Improve compile time Dec 9, 2020
@bors bors bot closed this Dec 9, 2020
@michaelsproul
Copy link
Member Author

Fiiiiiinally

@michaelsproul michaelsproul deleted the compile-time branch December 9, 2020 02:54
@michaelsproul michaelsproul mentioned this pull request Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants