Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.08k stars 107 forks source link

Track `Cargo.lock` files to improve CI speed #295

Closed escritorio-gustavo closed 5 months ago

escritorio-gustavo commented 6 months ago

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

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 commented 6 months ago

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 commented 6 months ago

Seems like a7cc7da just broke caching again :/

escritorio-gustavo commented 6 months ago

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 commented 6 months ago

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

escritorio-gustavo commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

NyxCode commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Any idea what's slow here?

Sorry, I've got clue either

NyxCode commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

CI is now down to 49 seconds!

escritorio-gustavo commented 6 months ago

making all the tests into a single test binary.

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

escritorio-gustavo commented 6 months ago

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

NyxCode commented 5 months ago

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

gustavo-shigueo commented 5 months ago

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 commented 5 months ago

I think I can do something better though...

/tests
|--- main.rs
|--- /integration
|------- arrays.rs
|------- generics.rs
|------- other_tests.rs
escritorio-gustavo commented 5 months ago

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

escritorio-gustavo commented 5 months ago

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

NyxCode commented 5 months ago

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 commented 5 months ago

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