stable rust toolchain

[aatifsyed]

Summary of changes

Changes introduced in this pull request:

• Switch to stable rust toolchain
• Shim nightly for rustdoc (to generate an index page for self-hosted rustdoc)
• Drop rustfmt config (we included nightly-only features)
• Minor code changes to fixup formatting and unknown lints
• Drop most custom compile flags from /cargo/config.toml
• Surgical update of dependencies that we need to update to run on stable

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

• I have performed a self-review of my own code,
• I have made corresponding changes to the documentation,
• I have added tests that prove my fix is effective or that my feature works (if possible),
  • N/A
• I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

[aatifsyed]

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

[LesnyRumcajs]

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.

[aatifsyed]

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

• chore: custom cargo config to reduce build time #2104 Added this https://github.com/ChainSafe/forest/pull/2104/files#diff-9a4f3e4537ebb7474452d131b0d969d89a51286f4269aac5ef268e712be17268R19
• chore: remove lld from system package deps #2200 Removes the link arg that actually speeds up linking, leaving us with technical debt in our build args 'linker - "clang"' that we're not sure works

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

[LesnyRumcajs]

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?

[lemmih]

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

[LesnyRumcajs]

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.

[aatifsyed]

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.

[elmattic]

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

[elmattic]

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.

[elmattic]

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

[LesnyRumcajs]

Thanks for assessing this, LGTM.

[LesnyRumcajs]

Dismissing review so it's not blocked

[aatifsyed]

We've pinned to fvm 2.3.x ChainSafe/fil-actor-states#102
We need fvm 2.4.x to take this cross-compilation fix: filecoin-project/rust-fil-proofs#1679
Otherwise cross compilation fails: https://github.com/ChainSafe/forest/actions/runs/5008225935/jobs/8975856604?pr=2876#step:9:2115

We should do whatever work we need to to take the latest fvm 2.x.x release.

[hanabi1224]

I find these grouping rules pretty helpful. Any ideas on keeping the functionalities after the change?

[aatifsyed]

It would be nice, but not possible on stable - would be pretty easy to write a custom linter, but probably not a priority right now

[aatifsyed]

(see #2876 (comment))

Hubert has said "LGTM"

[lemmih]

Nice!