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

Speedup CI #5691

Merged
merged 1 commit into from
Jul 2, 2023
Merged

Speedup CI #5691

merged 1 commit into from
Jul 2, 2023

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented May 15, 2023

Motivation

tokio CI is quite slow.

Solution

  • Use cargo-nextest to speedup testing
  • Use cargo-check instead of cargo-build and cargo test --no-run.
  • Run cargo-clippy with --no-deps
  • Set CARGO_UNSTABLE_SPARSE_REGISTRY to true so that nightly toolchain would use sparse crates.io registries
  • Adjust order of steps to avoid caching crates installed by taiki-e/install-action@v2
  • Use Swatinem/rust-cache@v2 in job no-atomic-u64
  • Add concurrenty group to cancel outdated CI

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

@Darksonn Darksonn requested a review from taiki-e May 15, 2023 11:57
@Darksonn Darksonn added the A-ci Area: The continuous integration setup label May 15, 2023
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented May 15, 2023

(I canceled old CI runs on this PR)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented May 15, 2023

Also, the arm-unknown-linux-gnueabihf1 in cross-test probably needs to be changed to armv5te-unknown-linux-gnueabi. It seems it was for testing platforms without AtomicU64, which is probably incorrect because arm-unknown-linux-gnueabihf has AtomicU64. ARM targets that do not have AtomicU64 are armv5te-unknown-linux-{gnu,musl,uclibc}eabi and arm-linux-androideabi (and some bere-metal targets).

- target: arm-unknown-linux-gnueabihf
- target: armv7-unknown-linux-gnueabihf
- target: aarch64-unknown-linux-gnu
rustflags: --cfg tokio_taskdump
# Run a platform without AtomicU64 and no const Mutex::new
- target: arm-unknown-linux-gnueabihf

Footnotes

  1. setup-cross-toolchain-action does not currently support it. cross-rs supports it but has been miscompiled for a long time -- miscompilation has been fixed, but support for this target has some limitations.

@NobodyXu
Copy link
Contributor Author

Also, the arm-unknown-linux-gnueabihf1 in cross-test probably needs to be changed to armv5te-unknown-linux-gnueabi. It seems it was for testing platforms without AtomicU64, which is probably incorrect because arm-unknown-linux-gnueabihf has AtomicU64. ARM targets that do not have AtomicU64 are armv5te-unknown-linux-{gnu,musl,uclibc}eabi and arm-linux-androideabi (and some bere-metal targets).

Thank you, I've updated PR as you requested.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented May 15, 2023

BTW, the performance of Valgrind-related tests is currently unstable and sometimes takes a long time, but I guess this should be fixed by passing --fair-sched=yes to Valgrind.

run: valgrind --error-exitcode=1 --leak-check=full --show-leak-kinds=all ./target/release/examples/${{ matrix.stress-test }}

run: valgrind --error-exitcode=1 --leak-check=full --show-leak-kinds=all ./target/debug/test-mem

run: valgrind --error-exitcode=1 --leak-check=full --show-leak-kinds=all ./target/debug/test-process-signal

@NobodyXu
Copy link
Contributor Author

NobodyXu commented May 18, 2023

BTW, the performance of Valgrind-related tests is currently unstable and sometimes takes a long time, but I guess this should be fixed by passing --fair-sched=yes to Valgrind.

Thanks, I've added this as instructed and it indeeds runs much faster.

@NobodyXu NobodyXu requested a review from taiki-e May 18, 2023 11:51
@NobodyXu
Copy link
Contributor Author

According to my observation of recent CI runs, this PR reduces workflow ci.yml from ~30m => ~18m

@NobodyXu
Copy link
Contributor Author

@taiki-e I've rebased this PR and removed the unstable sparse registries environment variable

@NobodyXu
Copy link
Contributor Author

cc @taiki-e @carllerche This PR has been stale for quite some time.
Can you review this please?

Thanks!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/stress-test.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@NobodyXu NobodyXu requested a review from taiki-e May 27, 2023 14:52
@NobodyXu
Copy link
Contributor Author

@Darksonn Seems like disabling feature mio/log by default breaks CI wasm32-wasi

@Darksonn
Copy link
Contributor

Yes, see tokio-rs/mio#1676.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented May 29, 2023

Thanks @Darksonn !

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jun 3, 2023

@carllerche Newly introduced self-tuning and unstable::test_disable_lifo_slot seems to be quite slow when running on qemu, often stuck for a really long time.

Edit:

I think this is because qemu doesn't like multi-threaded program.

@taiki-e
Copy link
Member

taiki-e commented Jun 11, 2023

It seems test on i686 with Mutex-based AtomicU64 is hanging (it does not use QEMU): https://github.com/tokio-rs/tokio/actions/runs/5222355294/jobs/9427778431?pr=5691

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jun 11, 2023

It seems test on i686 with Mutex-based AtomicU64 is hanging (it does not use QEMU): https://github.com/tokio-rs/tokio/actions/runs/5222355294/jobs/9427778431?pr=5691

So we don't need that taiki-e/setup-cross-toolchain-action@v1 in no-atomic-test?

I would try removing that.

@taiki-e
Copy link
Member

taiki-e commented Jun 11, 2023

So we don't need that taiki-e/setup-cross-toolchain-action@v1 in no-atomic-test?

No, it installs linker and libraries for i686 as the comment says.

# Install linker and libraries for i686-unknown-linux-gnu
- uses: taiki-e/setup-cross-toolchain-action@v1

@NobodyXu
Copy link
Contributor Author

So we don't need that taiki-e/setup-cross-toolchain-action@v1 in no-atomic-test?

No, it installs linker and libraries for i686 as the comment says.

# Install linker and libraries for i686-unknown-linux-gnu
- uses: taiki-e/setup-cross-toolchain-action@v1

Thanks!
I've removed it from no-atomic-check though since it only runs cargo-check and also removed RUST_TEST_THREADS=1 there since it's not run using qemu.

@taiki-e
Copy link
Member

taiki-e commented Jun 11, 2023

It seems test on i686 with Mutex-based AtomicU64 is hanging (it does not use QEMU): https://github.com/tokio-rs/tokio/actions/runs/5222355294/jobs/9427778431?pr=5691

We previously used --test-threads=1 for this test, so I think it is possible that the removal of that may have caused the problem.

- run: cargo test -Zbuild-std --target target-specs/i686-unknown-linux-gnu.json -p tokio --all-features -- --test-threads 1 --nocapture

@NobodyXu
Copy link
Contributor Author

We previously used --test-threads=1 for this test, so I think it is possible that the removal of that may have caused the problem.

- run: cargo test -Zbuild-std --target target-specs/i686-unknown-linux-gnu.json -p tokio --all-features -- --test-threads 1 --nocapture

Ok, I've added it back.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented Jun 12, 2023

Ok, I've added it back.

Thanks, this seems to fix the problem: https://github.com/tokio-rs/tokio/actions/runs/5235492644/jobs/9452384654?pr=5691

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @NobodyXu!

@NobodyXu
Copy link
Contributor Author

CI "Test hyper (macos-latest)" failed due to:

thread 'http2_keep_alive_count_server_pings' panicked at 'timed out waiting for pings: Elapsed(())', tests/server.rs:2488:10

looks like just a temporary network failure.

@taiki-e
Copy link
Member

taiki-e commented Jun 12, 2023

Hmm, is nextest not handling custom targets correctly? In addition to the following warning, all dependencies are recompiled when doctest is built.

warning: could not determine target triple: error deserializing target triple from --target option
caused by: unknown platform triple
caused by: unknown triple string: target-specs/i686-unknown-linux-gnu.json
caused by: Unrecognized architecture: target

https://github.com/tokio-rs/tokio/actions/runs/5244982846/jobs/9472222567#step:7:1274

Although it probably wouldn't be difficult for nextest to add support for custom targets (probably basically just porting the logic around this to nextest-runner).

@NobodyXu
Copy link
Contributor Author

@taiki-e I opened a feature request in nextest nextest-rs/nextest#887

@NobodyXu
Copy link
Contributor Author

@taiki-e cargo-nextest v0.9.54 has released which support custom target.

I triggered the PR before new release of install-action is cut which didn't include cargo-nextest v0.9.54

can you re-run the tests so that it uses latest nextest?

@NobodyXu
Copy link
Contributor Author

Merge conflict resolved, this PR is now mergeable.

 - Pass `--no-deps` to `cargo-clippy`
 - Use `dtolnay/rust-toolchain@stale` instead of
   `dtolnay/rust-toolchain@master`
 - Use dtolnay/rust-toolchain instead of `rustup` directly
 - Use `cargo-nextest` in job test to speedup testing
 - Use `cargo-nextest` in job test-unstable to speedup testing
 - Use `cargo-nextest` in job test-unstable-taskdump to speedup testing
 - Use `cargo-nextest` in job no-atomic-u64 to speedup testing
 - Use `cargo-nextest` in job check-unstable-mt-counters
 - Run `cargo check --benches` for `benches/` in job test
   Since the benchmark is not run
 - Run `cargo-check` instead of `cargo-build` in job test-parking_lot
   since no test is run
 - Run `cargo-check` instead of `cargo-build` in job no-atomic-u64
 - Run `Swatinem/rust-cache@v2` after `taiki-e/install-action@v2` to
   avoid caching pre-built artifacts downloaded by it.
 - Use `Swatinem/rust-cache@v2` in job no-atomic-u64
 - Add concurrenty group to cancel outdated CI
 - Use `taiki-e/setup-cross-toolchain-action@v1` in job cross-test

   instead of cross, so that we can use `cargo-nextest` to run tests in
   parallel.

   Also use `Swatinem/rust-cache@v2` to cache artifacts.
 - Use `Swatinem/rust-cache@v2` in job cross-check to speedup ci.
 - Fix job `cross-test`: Use `armv5te-unknown-linux-gnueabi` for no-atomic-u64

   testing instead of `arm-unknown-linux-gnueabihf`, which actually has
   atomic-u64
 - Rm use of `cross` in job `cross-check`

   Since it does not run any test, it does not need the `cross-rs`
   toolchain as tokio does not use any external C/C++ library that require
   `gcc`/`clang` to compile.
 - Add more recognizable name for steps in job cross-test
 - Split job `test` into `test-{tokio-full, workspace-all-features,
   integration-tests-per-feature}`
 - Split job `no-atomic-u64` into `no-atomic-u64-{test, check}`
 - Parallelize job `features` by using matrix
 - Split `cross-test` into `cross-test-{with, without}-parking_lot`
 - Speedup job `cross-test-*` and `no-atomic-u64-test` by running
   `cargo-test` with `-- --test-threads 1` since `qemu` userspace
   emulation has problems running binaries with many threads.
 - Speedup workflow `stress-test.yml` and job `valgrind` in workflow `ci.yml`
   by passing `--fair-sched=yes` to `valgrind`.
 - Speedup job `test-hyper`: Cache `./hyper/target`
   instead of caching `./target`, which is non-existent.
 - Set `RUST_TEST_THREADS=1` to make sure `libtest` only use one thread
   so that qemu will be happy with the tests.
   This is applied to `cross-test-with(out)-parking_lot, no-atomic-u64-test`.
 - Apply suggestions from code review

Co-authored-by: Taiki Endo <te316e89@gmail.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@taiki-e taiki-e merged commit fc69666 into tokio-rs:master Jul 2, 2023
@taiki-e
Copy link
Member

taiki-e commented Jul 2, 2023

Thanks!

@NobodyXu NobodyXu deleted the speedup-ci branch July 2, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: The continuous integration setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants