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

Track Cargo.lock files to improve CI speed #295

Merged
merged 29 commits into from
Apr 15, 2024
Merged

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Apr 3, 2024

Goal

In #258 we added caching to our CI to reduce the time it takes to run. In the rust-cache docs it's mentioned that having the Cargo.lock files on GitHub improves the cache's effectiveness, and from what I have seen from the 2 last CI runs in this branch, CI running time has been cut down by a full minute about 20-30 seconds, (the one minute difference was caused by the initial commit being slower than usual).

For some reason, the log message gets stuck like this

Run cargo test --all-features
    Updating crates.io index
   Compiling ts-rs-macros v8.1.0 (/home/runner/work/ts-rs/ts-rs/macros)
   Compiling ts-rs v8.1.0 (/home/runner/work/ts-rs/ts-rs/ts-rs)
   Compiling example v0.1.0 (/home/runner/work/ts-rs/ts-rs/example)

for a lot of time in the --all-features test, even though all the dependencies are already compiled

Changes

  • Remove Cargo.lock from .gitignore
  • Add cargo test profile to prioritize build time
  • Replace actions-rs/toolchain with dtolnay/rust-toolchain
  • Combine all integration tests into a single binary by moving each test file to its own directory and creating tests/main.rs

Sources

https://fasterthanli.me/articles/why-is-my-rust-build-so-slow
https://corrode.dev/blog/tips-for-faster-rust-compile-times/#combine-all-integration-tests-into-a-single-binary
https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

@escritorio-gustavo
Copy link
Contributor Author

For some reason, the log message gets stuck like this

Run cargo test --all-features
    Updating crates.io index
   Compiling ts-rs-macros v8.1.0 (/home/runner/work/ts-rs/ts-rs/macros)
   Compiling ts-rs v8.1.0 (/home/runner/work/ts-rs/ts-rs/ts-rs)
   Compiling example v0.1.0 (/home/runner/work/ts-rs/ts-rs/example)

for a lot of time in the --all-features test, even though all the dependencies are already compiled

Running cargo test --all-features locally after a cargo clean, it seems that each individual test file takes a pretty long time to build

@escritorio-gustavo
Copy link
Contributor Author

Seems like a7cc7da just broke caching again :/

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 3, 2024

it seems that each individual test file takes a pretty long time to build

cargo test --all-features --timings confirms this. The generics.rs file alone takes 12.7 seconds!

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Wait, what? Thats wild, I'll have to look into that.
These excessive build times are just for --all-features?

@escritorio-gustavo
Copy link
Contributor Author

These excessive build times are just for --all-features?

Not really, but it's much worse for --all-features

With just cargo test --timings I get 7.54 seconds for the generics.rs test file

@escritorio-gustavo
Copy link
Contributor Author

The annoying thing is that it's not consistent, for instance, I just ran the --all-features again and got 4.86s for generics.rs

@escritorio-gustavo
Copy link
Contributor Author

image
This is a screenshot of the graph on my latest run, filtering on >= 5s

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Any idea what's slow here?
As far as I can tell, the actual macro invocations are fast - No single #[derive(TS)] seems to take more than 800µs - most are around 200. The type-checking for TypeList is my next suspect, but I have no idea how to get to the bottom of that.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Even something like union.rs takes 2s (clean build, ofc) on my machine - there's nothing in there, just two structs, and no complicated TypeLists.

@escritorio-gustavo
Copy link
Contributor Author

The type-checking for TypeList is my next suspect, but I have no idea how to get to the bottom of that.

If that's the case, maybe merging #293 will make it faster?

@escritorio-gustavo
Copy link
Contributor Author

Any idea what's slow here?

Sorry, I've got clue either

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Some things got faster with #293, but overall, cargo clean && cargo build --tests --timings reports 19.6s vs 19.4s.
So not the typelists then either? I'm confused.

@escritorio-gustavo
Copy link
Contributor Author

Even something like union.rs takes 2s (clean build, ofc) on my machine - there's nothing in there, just two structs, and no complicated TypeLists.

So... I removed all mentions to TS in union.rs and still got 3.3s lol

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Yeah, I think you're onto something. It seems to be just the tests that are painfully slow. I'm not familiar with the internals, but there's some machinery around rust tests, so maybe that makes them compile very slowly?

A clean debug build of our example takes 0.45s on my machine, but 1.93s with --tests.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 4, 2024

I have managed to get test compile times way down by changing the test profile and making all the tests into a single test binary. I'll push it here and check if CI gets faster.

This was done by moving all tests from tests/foo.rs to tests/foo/mod.rs and adding a tests/main.rs file with mod foo; inside

@escritorio-gustavo
Copy link
Contributor Author

CI is now down to 49 seconds!

@escritorio-gustavo
Copy link
Contributor Author

making all the tests into a single test binary.

This single binary compiles in 8-10 seconds on my machine

@escritorio-gustavo
Copy link
Contributor Author

CI is now under one minute and no longer emits any deprecation warnings

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 8, 2024

Is there a reason why all the tests had to be moved into their own directory?

ts-rs/Cargo.toml Outdated Show resolved Hide resolved
@gustavo-shigueo
Copy link
Collaborator

gustavo-shigueo commented Apr 9, 2024

Is there a reason why all the tests had to be moved into their own directory?

Any file directly inside the tests directory will be compiled into its own binary, which is very slow because of the linker.

By doing this, only one binary is created (tests/main.rs) and each of the other files is just a mod belonging to main, this means the linker is only invoked once

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 9, 2024

I think I can do something better though...

/tests
|--- main.rs
|--- /integration
|------- arrays.rs
|------- generics.rs
|------- other_tests.rs

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 10, 2024

Small update: CI is now in the range of 40s-1min

@escritorio-gustavo
Copy link
Contributor Author

Hey @NyxCode, what do you think of the current state of this PR?

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 12, 2024

I'm very happy with this, thanks! CI stuff always gives me headaches, so I try to avoid dealing with it ^^

Is there anything we should keep in mind now that we've got the Cargo.lock files in the repo?
Maybe it'd be enough to regularly run a cargo update? IIrc, dprint_plugin_typescript did some breaking changes in a patch release in the past.

@escritorio-gustavo
Copy link
Contributor Author

Is there anything we should keep in mind now that we've got the Cargo.lock files in the repo?

I don't think there's much to worry about other than, as you said, running cargo update every now and then

IIrc, dprint_plugin_typescript did some breaking changes in a patch release in the past.

We'd be succeptible to this anyway, in the previous setup, due to the lack of a Cargo.lock, CI would always use the latest patch version

@escritorio-gustavo escritorio-gustavo merged commit 1ae4148 into main Apr 15, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the cargo_lock branch April 15, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants