feat: move to cargo-xwin for clippy

[samypr100]

Summary

Move to cargo-xwin for clippy

Closes #3507

[messense]

FYI, consider install cargo-xwin from pypi or github releases to avoid compiling it. And you should try to cache Windows SDK download by explicitly setting XWIN_CACHE_DIR and cache it via the cache action.

[charliermarsh]

Would cargo-binstall work or no?

[messense]

probably works, I never tried.

[messense]

BTW, I think you forgot to use a Linux runner? otherwise using cargo-xwin on Windows isn't beneficial?

[samypr100]

Indeed, was trying to gather some baseline data on windows first 😅; numbers don't look too promising thus far.

[codspeed-hq]

CodSpeed Performance Report

Merging #3600 will not alter performance

_{Comparing samypr100:cargo-xwin-clippy (aa61db7) with main (ed91b1d)}

Summary

✅ 12 untouched benchmarks

[konstin]

The clippy step is 12:30min while clippy itself takes 30s, do you know where the other 12min are? The xwin download and unpack are <2min on my machine.

[messense]

do you know where the other 12min are?

Probably because the network connection from GH action to Windows SDK download isn't very good, cached version is much better.

[konstin]

Clippy itself takes 30s, the whole job 1:30min, great work!

[samypr100]

😅 need to add up the totals numbers and make a table, will do later today to get better comparison.

At a glance on an unchached run it's marginally worse by a quite the factor (2m-3m before vs 15 min now) ~ .
On cached runs job totals are actually about the same on both windows and linux.

TL;DR Seems when cache is busted the impact is much worse than it is now and we're not gaining on job total speeds.

(will also explore some other fine tuning later like separating the windows sdk cache out)

[konstin]

Would moving the xwin cache into a separate cache step only keyed on rust-toolchain.toml help? This way the cache would only ever be invalidated when we upgrade rust

[samypr100]

Would moving the xwin cache into a separate cache step only keyed on rust-toolchain.toml help? This way the cache would only ever be invalidated when we upgrade rust

You read my mind 😆

[zanieb]

We do frequently fill our cache, we'll want to be careful that it's not evicted often due to space limitations.

[samypr100]

Here's some of the initial numbers to compare I've grabbing from all the past runs.

Current Main	Before Cache (Step, Job)	After Cache (Step, Job)
Clippy on Windows	2m~, 3m~	30s~, 1.25m~

New Clippy Runs	After (Step, Job)
xwin on windows	2m 45s~, 5m~
xwin on linux (no cache)	14m~, 15m~
xwin on linux (rust cache)	12m~, 13m~
xwin on linux (xwin cache)	2m~, 3m~
xwin on linux (rust + xwin cache)	30s~, 1.25m~

It seems clippy xwin runs about the same speed as windows currently (post the ReFS move). The job totals are aligned on best scenarios.

The xwin cache made the biggest difference for cargo xwin, making it run consistently with current windows runner, but it introduces a bigger risk when that cache gets busted.

The 15 minute hit will be most noticeable on downstream forks (where there is no cache often) or when cache is lost in main after rust toolchain upgrades or cache eviction.

[konstin]

Personally, i think this still is a noticeable improvement, it means we skip the virtual disk setup and have a windows runner less; the 15min cache priming about every 6 weeks are acceptable.

[messense]

I don't think xwin cache needs to be keyed by Rust version, it's just an unpacked Windows SDK, no Rust involved.

[samypr100]

Thanks, moved cache key to be just cargo-xwin

[samypr100]

Personally, i think this still is a noticeable improvement, it means we skip the virtual disk setup and have a windows runner less; the 15min cache priming about every 6 weeks are acceptable.

Sounds good, PR is good to go now. Apologies, I thought I had this PR was in draft all this time 😅