Make dogfood tests run in `release` mode #10576

blyxyas · 2023-03-31T13:35:32Z

The old PR was about creating a new alias in .cargo/config.toml. After some extra testing we noticed that the speedup wasn't that critical and @flip1995 created a new patch to actually make dogfood tests run with a --release argument.

Ignore any conversation before this comment, as the PR has been re-done.

changelog:none

rustbot · 2023-03-31T13:35:38Z

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

flip1995 · 2023-03-31T13:52:07Z

How long does dev-r take from a clean state compared to just dev?

Alexendoo · 2023-03-31T13:57:10Z

I wouldn't expect this to have such a large effect on the times since cargo dev dogfood is just handing off to a cargo test invocation, running

cargo dev dogfood
touch clippy_lints/src/lib.rs
time cargo dev dogfood

cargo dev-r dogfood
touch clippy_lints/src/lib.rs
time cargo dev-r dogfood

Gives me ~40s for both times

flip1995 · 2023-03-31T14:22:15Z

Outdated

Note: dogfood doesn't do anything when you run the command twice without changing anything. Maybe that's where the improvement is coming from.

I ran cargo clean && time cargo dev dogfood 3x:

No changes, all debug

cargo dev dogfood  319.60s user 37.20s system 419% cpu 1:25.11 total

Your changes

cargo dev dogfood  368.70s user 39.13s system 450% cpu 1:30.47 total

With the patch below

cargo dev dogfood  208.53s user 21.94s system 513% cpu 44.853 total

Patch:

diff --git a/clippy_dev/src/dogfood.rs b/clippy_dev/src/dogfood.rs
index b69e9f649ec..35653c76cc9 100644
--- a/clippy_dev/src/dogfood.rs
+++ b/clippy_dev/src/dogfood.rs
@@ -12,6 +12,10 @@ pub fn dogfood(fix: bool, allow_dirty: bool, allow_staged: bool) {
         .args(["--features", "internal"])
         .args(["--", "dogfood_clippy"]);

+    if !cfg!(debug_assertions) {
+        cmd.arg("--release");
+    }
+
     let mut dogfood_args = Vec::new();
     if fix {
         dogfood_args.push("--fix");

Another experiment: When I already ran cargo build to build Clippy and clippy_dev and then run time cargo dev dogfood (assuming the above patch was applied):

Just dev:

cargo dev dogfood  301.85s user 34.24s system 396% cpu 1:24.70 total

With dev-r:

cargo dev dogfood  180.30s user 18.08s system 491% cpu 40.341 total

With dev but the --release flag applied unconditionally:

cargo dev dogfood  165.80s user 20.05s system 472% cpu 39.327 total

So I think the best thing to do here would be:

diff --git a/clippy_dev/src/dogfood.rs b/clippy_dev/src/dogfood.rs
index b69e9f649ec..484e0d4e88b 100644
--- a/clippy_dev/src/dogfood.rs
+++ b/clippy_dev/src/dogfood.rs
@@ -10,7 +10,8 @@ pub fn dogfood(fix: bool, allow_dirty: bool, allow_staged: bool) {
     cmd.current_dir(clippy_project_root())
         .args(["test", "--test", "dogfood"])
         .args(["--features", "internal"])
-        .args(["--", "dogfood_clippy"]);
+        .args(["--", "dogfood_clippy"])
+        .arg("--release");

     let mut dogfood_args = Vec::new();
     if fix {

and NOT introduce dev-r.

blyxyas · 2023-03-31T14:22:41Z

Outdated

I just tested again with a clean slate of a recently pulled `master`. It is ~2 seconds faster. The first testing environment was in a mid-debugging branch, so dogfood was failing faster, that's why I found it so useful.

In a "stable" and after cargo clean, it looses all hopes to be useful. So I guess this PR is useless and should be closed.

---

I just saw flip1955's latest comment; going to test it.
So should the PR be closed, or just completely re-done?

flip1995 · 2023-03-31T14:30:17Z

So should the PR be closed, or just completely re-done?

You can either open a new PR or redo this one. Up to you.

flip1995

I would approve, but since this is my own patch, I don't want to approve my own thing.

flip1995 · 2023-03-31T14:35:22Z

Please update the PR description+title before this gets merged though

Alexendoo · 2023-03-31T17:31:36Z

That cuts the runtime down by not running it :P

% cargo test --test dogfood --features internal -- dogfood_clippy --release
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running tests/dogfood.rs (target/debug/deps/dogfood-bf1f6a65afcf8f1b)
error: Unrecognized option: 'release'
error: test failed, to rerun pass `--test dogfood`

That doesn't get shown by cargo dev dogfood though because it only prints the stdout, would be best to change that to inherit stdout + stderr rather than using .output() + println

Jarcho · 2023-03-31T17:56:02Z

This is probably not worth it. Running dogfood in release mode would mean compiling clippy in release mode, which basically negates all the speed gains from running clippy on itself faster. Basically more than double the compilation time for release mode and dogfood now has to run in zero time.

It also means getting an ICE while running dogfood will be much less pleasant experience.

blyxyas · 2023-03-31T18:24:38Z

Which basically negates all the speed gains from running clippy on itself faster.

It seems like it doesn't (only compiling Clippy):

dev: time cargo test --test dogfood --features internal -- dogfood_clippy: 1:35.78 total
release: time cargo test --test dogfood -r --features internal -- dogfood_clippy: 1:10.50 total

On the topic of the unrecognized option, moving --release to before --features is declared seems to work.

- cargo test --test dogfood --features internal -- dogfood_clippy --release

+ cargo test --test dogfood --release --features internal -- dogfood_clippy

Jarcho · 2023-03-31T20:23:45Z

Just tested on my machine:

cargo test --test dogfood --features internal with a single change to clippy_lints compiles in 0:20 and takes 1:20 total.
cargo test --test dogfood --release --features internal with a single change to clippy_lints compiles in 1:25 and takes 1:58 total.

Both tests were don first by running the command, then changing clippy_lints by adding a newline to the start of lib.rs. I haven't been able to make debug mode slower overall even when making actual functional changes to the code either, though that does close the gap a bit. Any change to clippy_utils makes the gap significantly wider.

I'll run some more tests later to try different scenarios.

Jarcho · 2023-04-01T02:10:03Z

More perf tests on windows:

For debug mode with incremental:

cargo clean && cargo build --all-targets --features internal
Add newline to start of clippy_lints/src/lib.rs (seems to cause full recompile)
measure-command { cargo test --test dogfood --features internal }

Time: 3:13

cargo clean && cargo build --all-targets --features internal
Add newline to start of clippy_lints/src/allow_attributes.rs
measure-command { cargo test --test dogfood --features internal }

Time: 2:25

For debug mode without incremental:

cargo clean && cargo build --all-targets --features internal
add newline to start of clippy_lints/src/lib.rs
measure-command { cargo test --test dogfood --features internal }

Time: 2:41

For release mode:

cargo clean && cargo build --all-targets --features internal --release
add newline to start of clippy_lints/src/lib.rs
measure-command { cargo test --test dogfood --features internal --release }

Time: 3:02

Release mode is only beating incremental debug mode when none of the incremental cache seems to get used.

blyxyas · 2023-04-01T09:42:11Z

This differences maybe are caused by platform differences?

These are my times (Using the same commands):

DEBUG	Blyxyas' times (Linux)	Jarcho's times (Windows)
Change to `lib.rs`	2:02	3:13
Change to `lib.rs` (No incremental)	2:03	2:41
Change to `allow_attribute.rs`	1:34	2:25
RELEASE
Change to `lib.rs` (Incremental)	1:40.93	3:02

Difference from Linux lib.rs changes (incremental) to release: -18.0%
Difference from Windows lib.rs changes (incremental) to release: -5.6%

Difference from Linux lib.rs changes (No incremental) to release: -18.7%
Difference from Windows lib.rs changes (No incremental) to release: +13%

So this PR makes sense only for Linux users.

Note that, a "mid-debugging" scenario (adding a new line to allow_attribute.rs) does increase dogfood times (In my case, by +4%). I don't think this is significant, as the command for quickly testing (for debugging) a lint is TESTNAME=... cargo uitest, not cargo test or cargo dev dogfood

blyxyas · 2023-04-01T09:59:24Z

If someone could verify the results on their machine. Maybe test them on Mac OS?

flip1995 · 2023-04-03T11:49:22Z

That cuts the runtime down by not running it :P

That's not the outcome we want. I should have checked for that 😅

In that case, compiling Clippy in release mode and running dogfood in release mode vs both in debug mode took about the same time for me. So I don't think this improves things significantly. Also since it is really machine dependent as it seems, I don't think adding a new alias for it is worth it.

Alexendoo · 2023-04-03T13:25:30Z

On mine, with

cargo clean
time cargo test --test dogfood --features internal
touch clippy_lints/src/lib.rs
time cargo test --test dogfood --features internal

cargo clean
time cargo test --test dogfood --features internal --release
touch clippy_lints/src/lib.rs
time cargo test --test dogfood --features internal --release

	Linux	Windows
Debug, clean	3:49	1:50
Debug, rerun	1:20	0:39
Release, clean	4:23	1:48
Release, rerun	1:58	0:50

flip1995 · 2023-04-03T13:41:45Z

With this patch, same commands as #10576 (comment)

	`blyxyas:dev_release`	`master`
Debug, clean	1:18	1:19
Debug, rerun	0:32	0:25
Release, clean	1:22	1:23
Release, rerun	0:40	0:40

So I would conclude that this PR doesn't improve the situation.

EDIT: I use Arch Linux btw

CPU: 11th Gen Intel Core i7-11800H @ 16x 4.6GHz

blyxyas · 2023-04-03T13:42:47Z

So Alexendoo's differences are:

LINUX
Debug clean - Release clean	+14%
Debug re-run - Release re-run	+47.5%
WINDOWS
Debug clean - Release clean	-1.8%
Debug re-run - Release re-run	+28.2%

So Alexendoo's machine also performes worse, even on Linux.
Seeing that this is extremely machine-dependant, and seeing that it'd take a lot of time to investigate the origin of these differences (number of cores maybe? Maybe a hidden architecture differences? Maybe brand differences between Intel and AMD?) and I'm not even sure conditional compilation could solve the issue, I think this PR should be closed.

(Please react if you agree or disagree)

flip1995 · 2023-04-03T14:17:37Z

Thanks for investigating this!

Build quine-mc_cluskey with `opt-level=3` in dev builds While doing some profiling I noticed that debug clippy running on the `clippy_lints` crate spends 35s out of 160s in one specific code path of `nonminimal_bool`, which seemed a bit excessive. I've found that just enabling optimizations for quine-mc_cluskey (used by nonminimal_bool) cuts down the part that took 35s to 3s While this doesn't really change anything for users, this helps dogfood a bit as it cuts off about half a minute of runtime (in some of my tests, at least). Something similar was attempted in #10576, however that involved compiling everything in release mode including clippy itself, whereas this only affects a single dependency that's compiled in parallel with something that takes longer so this should hopefully not have a negative impact in any case (and changing clippy doesn't require recompiling that dependency) changelog: none

rustbot assigned Alexendoo Mar 31, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 31, 2023

blyxyas changed the title ~~Add alias dev-r~~ Make dogfood tests run in release mode Mar 31, 2023

flip1995 approved these changes Mar 31, 2023

View reviewed changes

Make dogfood tests run in release mode

8a18e40

blyxyas force-pushed the dev_release branch from 4be587b to 8a18e40 Compare March 31, 2023 14:34

blyxyas added 2 commits March 31, 2023 20:27

Move -r flag forwards

b2a0f33

Print stdout and stderr instead of just command output

28d13f4

Add conditional compilation for Windows cases

b997b60

blyxyas closed this Apr 3, 2023

y21 mentioned this pull request Sep 17, 2024

Build quine-mc_cluskey with opt-level=3 in dev builds #13408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make dogfood tests run in `release` mode #10576

Make dogfood tests run in `release` mode #10576

blyxyas commented Mar 31, 2023 •

edited

Loading

rustbot commented Mar 31, 2023

flip1995 commented Mar 31, 2023

Alexendoo commented Mar 31, 2023

flip1995 commented Mar 31, 2023 •

edited

Loading

blyxyas commented Mar 31, 2023 •

edited

Loading

flip1995 commented Mar 31, 2023

flip1995 left a comment

flip1995 commented Mar 31, 2023

Alexendoo commented Mar 31, 2023

Jarcho commented Mar 31, 2023

blyxyas commented Mar 31, 2023 •

edited

Loading

Jarcho commented Mar 31, 2023 •

edited

Loading

Jarcho commented Apr 1, 2023

blyxyas commented Apr 1, 2023 •

edited

Loading

blyxyas commented Apr 1, 2023

flip1995 commented Apr 3, 2023

Alexendoo commented Apr 3, 2023

flip1995 commented Apr 3, 2023 •

edited

Loading

blyxyas commented Apr 3, 2023 •

edited

Loading

flip1995 commented Apr 3, 2023

Make dogfood tests run in release mode #10576

Make dogfood tests run in release mode #10576

Conversation

blyxyas commented Mar 31, 2023 • edited Loading

rustbot commented Mar 31, 2023

flip1995 commented Mar 31, 2023

Alexendoo commented Mar 31, 2023

flip1995 commented Mar 31, 2023 • edited Loading

blyxyas commented Mar 31, 2023 • edited Loading

flip1995 commented Mar 31, 2023

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Mar 31, 2023

Alexendoo commented Mar 31, 2023

Jarcho commented Mar 31, 2023

blyxyas commented Mar 31, 2023 • edited Loading

Jarcho commented Mar 31, 2023 • edited Loading

Jarcho commented Apr 1, 2023

blyxyas commented Apr 1, 2023 • edited Loading

blyxyas commented Apr 1, 2023

flip1995 commented Apr 3, 2023

Alexendoo commented Apr 3, 2023

flip1995 commented Apr 3, 2023 • edited Loading

blyxyas commented Apr 3, 2023 • edited Loading

flip1995 commented Apr 3, 2023

Make dogfood tests run in `release` mode #10576

Make dogfood tests run in `release` mode #10576

blyxyas commented Mar 31, 2023 •

edited

Loading

flip1995 commented Mar 31, 2023 •

edited

Loading

blyxyas commented Mar 31, 2023 •

edited

Loading

blyxyas commented Mar 31, 2023 •

edited

Loading

Jarcho commented Mar 31, 2023 •

edited

Loading

blyxyas commented Apr 1, 2023 •

edited

Loading

flip1995 commented Apr 3, 2023 •

edited

Loading

blyxyas commented Apr 3, 2023 •

edited

Loading