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.

Sign up for GitHub

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

Jump to bottom

stable rust toolchain #2876

Merged
merged 23 commits into from
May 24, 2023
Merged

stable rust toolchain #2876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
45483bf
mark: aatifsyed/stable-rust-toolchain
aatifsyed May 15, 2023
3d8cbfe
fix: use stable rust
aatifsyed May 15, 2023
1387877
fix: remove unknown clippy lints
aatifsyed May 15, 2023
d51613d
fix: include index.html for rustdoc
aatifsyed May 15, 2023
9cfc101
fix: remove rustfmt
aatifsyed May 15, 2023
8e41975
fix: cargo fmt
aatifsyed May 15, 2023
b88c850
fix: install toolchain for documenting
aatifsyed May 15, 2023
4f0104b
try: rustup install ... --component rustfmt
aatifsyed May 15, 2023
8e14772
try: get cargo version
aatifsyed May 15, 2023
2095eee
fix: command typo
aatifsyed May 15, 2023
4d04c29
tweak: remove avx2
aatifsyed May 16, 2023
3f525e2
tweak: add todos to cargo config
aatifsyed May 17, 2023
8b41bc1
fix: extraneous component added in CI
aatifsyed May 17, 2023
fcb6c4c
nomerge: demonstrate dependency
aatifsyed May 22, 2023
da7253e
Revert "nomerge: demonstrate dependency"
aatifsyed May 22, 2023
ef2ec11
try: cargo update -p fvm@2.3.0 -p fvm_shared@2.3.0
aatifsyed May 22, 2023
b0ad64b
Revert "try: cargo update -p fvm@2.3.0 -p fvm_shared@2.3.0"
aatifsyed May 22, 2023
b5837c8
try: (and fail) to just update what we want
aatifsyed May 22, 2023
758b0f6
fix: update lockfile
aatifsyed May 22, 2023
b2e0886
try: cargo update -p fvm@3.3.1
aatifsyed May 23, 2023
76def65
Generate Daily Benchmark Results (#2367)
jdjaustin May 23, 2023
3787c8f
review: tweaks
aatifsyed May 23, 2023
ec3f8c1
fix: take published fil_actors...
aatifsyed May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 4 additions & 32 deletions .cargo/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,10 @@
[alias]
b = "build"
c = "check"
d = "doc"
t = "test"
r = "run"
aatifsyed marked this conversation as resolved.
Show resolved Hide resolved

[build]
incremental = true
# might be helpful but we don't enforce sccache installation atm
# https://github.com/mozilla/sccache
# rustc-wrapper = "sccache"
aatifsyed marked this conversation as resolved.
Show resolved Hide resolved

# For details checkout https://jondot.medium.com/8-steps-for-troubleshooting-your-rust-build-times-2ffc965fd13e
[target.x86_64-unknown-linux-gnu]
# clang has been listed as prerequisite in the doc
linker = "clang"
Copy link
Member

Choose a reason for hiding this comment

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

What is the implication of removing this? I believe it was added to reduce linking time. If we still want to remove it, the dependencies in both README and Dockerfiles (and Makefile) should be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we own linking time? Or is it owned by the developer?

I'd argue that we don't own any cargo config:

  • Devs will have their own linkers set up in in ~/.cargo/config.toml. E.g if they've install mold
  • Fetching with CLI, cargo target directory etc, that's very developer-owned

The gray area here is --cfg=tokio_unstable, but I think that's a minor feature, and if users want to include it, we should provide instructions.

Note if we supported a devcontainer, we would then own more of the developer experience - linking building etc, but at the moment we're a mess of both (I guess this is why I'm spending so much time here).

We don't really want to be messing with build config too much, because it ends up being pain - we're not build experts, we're rust experts.
One sample is the vectorised instruction support which we've mentioned earlier. This is another excellent example

BTW this arg does nothing on my machine, you actually need a linker if you want a speedup:

  './do-link.bash -C link-arg=-fuse-ld=mold' ran
    1.02 ± 0.11 times faster than './do-link.bash -C linker=clang -C link-arg=-fuse-ld=mold'
    1.21 ± 0.08 times faster than './do-link.bash -C linker=clang -C link-arg=-fuse-ld=lld'
    1.22 ± 0.08 times faster than './do-link.bash -C link-arg=-fuse-ld=lld'
    4.81 ± 0.32 times faster than './do-link.bash -C linker=clang'
    4.95 ± 0.42 times faster than './do-link.bash'

See benchmarking at 54fb5e8

I'm generally all for build-time tuning, and developer experience, but I think the way we handle it at the moment is a bit haphazard, and we end up dropping balls outside of the actual source code

Copy link
Member

Choose a reason for hiding this comment

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

While the developer is free to tinker with his/her own setup, we do own the CI. The smaller the build times are, the more we can offload to GH runners and not paid buildjet machines which brings real benefits of reduced costs (and more subjective ones as well - we like to have green checkmarks sooner than later).

Thanks for doing the benchmark. I'd love to see mold or lld used in the CI.

All in all, do you agree that we should remove clang from dependencies if it is no longer used anywhere by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this motion. I'd have more confidence in the correctness of Forest if we used the default configuration.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I overall agree that using defaults is the way to go. That being said if a couple of lines of configuration that are rarely changed bring 10-20% faster builds for thousands of workflow runs in the future - IMO it's worth having it as long as it is not really exotic piece of technology that changes a lot.

Copy link
Contributor Author

@aatifsyed aatifsyed May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Impact of avx2

Methodology

I built the code with and without avx here: https://github.com/ChainSafe/forest/blob/c5b75126b84b1c5af98057187fe43a3f9a96cdff/build.bash
I ran the benchmark script here https://github.com/ChainSafe/forest/blob/c5b75126b84b1c5af98057187fe43a3f9a96cdff/benchmark.bash

  • a warmup run, followed by 20 runs, cleaning the db between each run
  • 100 tipsets for validation

I didn't have enough hard drive space to run against mainnet snapshots, so I used calibnet

Importing

Command Mean [s] Min [s] Max [s] Relative
avx2 36.508 ± 10.011 21.245 66.273 1.11 ± 0.36
no_avx2 32.762 ± 5.676 22.025 45.221 1.00

Validating

Command Mean [s] Min [s] Max [s] Relative
avx2 9.461 ± 0.126 9.149 9.739 1.00
no_avx2 9.860 ± 0.915 9.044 12.578 1.04 ± 0.10

Conclusion

There is no statistically significant difference between avx and non_avx builds.
This makes sense because at no point did I see we were CPU bound - I reckon we're io bound. Maybe if we were memory bound we'd get a speedup, but no user is going to map a ~100GB snapshot into memory, and our code is certainly not there yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have on my side:

Network: mainnet
Snapshot used: 2866560_2023_05_17T06_00_00Z.car
OS: Fedora 37
CPU: Intel Core i3-6300
Command Import Snapshot Validation of 30 heights
avx2 14m22s 2m16s
no_avx2 14m13s 2m12s

Copy link
Contributor

@elmattic elmattic May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you can confirm this on a mainnet snapshot as well, my CPU is pretty old (Skylake), probably worth testing on a recent intel gen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we can leave those instructions away, and re-assess performance later when needed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for assessing this, LGTM.

# enable avx2 by default since it's available on almost all x86_64 CPUs
rustflags = ["-Ctarget-feature=+avx2,+fma", "-Zshare-generics=y", "--cfg", "tokio_unstable"]

[target.x86_64-unknown-linux-musl]
# clang has been listed as prerequisite in the doc
linker = "clang"
# enable avx2 by default since it's available on almost all x86_64 CPUs
rustflags = ["-Ctarget-feature=+avx2,+fma", "-Zshare-generics=y", "--cfg", "tokio_unstable"]
aatifsyed marked this conversation as resolved.
Show resolved Hide resolved

[target.x86_64-apple-darwin]
# zld might help here
# brew install michaeleisel/zld/zld
# "-Clink-arg=-fuse-ld=zld"
# For details checkout https://jondot.medium.com/8-steps-for-troubleshooting-your-rust-build-times-2ffc965fd13e
rustflags = ["-Zshare-generics=y", "--cfg", "tokio_unstable"]

[target.aarch64-apple-darwin]
rustflags = ["-Zshare-generics=y", "--cfg", "tokio_unstable"]
# TODO(aatifsyed): remove - this can be pushed out to readme
# In all cases, pass --cfg=tokio_unstable for tokio console integration
# See (https://github.com/ChainSafe/forest/pull/2245)
rustflags = ["--cfg=tokio_unstable"]

[net]
git-fetch-with-cli = true
Expand Down
6 changes: 2 additions & 4 deletions .github/workflows/docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ on:
- main

env:
# https://doc.rust-lang.org/nightly/rustdoc/unstable-features.html#--index-page-provide-a-top-level-landing-page-for-docs
RUSTDOCFLAGS: "-Dwarnings -Zunstable-options --enable-index-page"
aatifsyed marked this conversation as resolved.
Show resolved Hide resolved
CACHE_TIMEOUT_MINUTES: 5
SCCACHE_ENDPOINT: ${{ vars.SCCACHE_ENDPOINT }}
SCCACHE_BUCKET: ${{ vars.SCCACHE_BUCKET }}
Expand Down Expand Up @@ -54,8 +52,8 @@ jobs:
RUSTFLAGS: "-Cstrip=symbols"
- name: Execute MDBook
run: make mdbook-build
- name: Build rustdoc
run: make rustdoc
- name: Build vendored docs
run: make vendored-docs
env:
CC: "sccache clang"
CXX: "sccache clang++"
Expand Down
5 changes: 0 additions & 5 deletions .rustfmt.toml
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These features have been unstable for a while, don't look likely to get stabilized soon (unless we e.g drive it...)

rust-lang/rustfmt#4991
rust-lang/rustfmt#3347
rust-lang/rustfmt#5081
rust-lang/rustfmt#5083

This file was deleted.

Loading