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

Sort tests at compile time, not at startup #99939

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jul 30, 2022

Recently, another Miri user was trying to run cargo miri test on the crate iced-x86 with --features=code_asm,mvex. This configuration has a startup time of ~18 minutes. That's ~18 minutes before any tests even start to run. The fact that this crate has over 26,000 tests and Miri is slow makes a lot of code which is otherwise a bit sloppy but fine into a huge runtime issue.

Sorting the tests when the test harness is created instead of at startup time knocks just under 4 minutes out of those ~18 minutes. I have ways to remove most of the rest of the startup time, but this change requires coordinating changes of both the compiler and libtest, so I'm sending it separately.

(except for doctests, because there is no compile-time harness)

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 30, 2022
@rust-highfive
Copy link
Collaborator

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

LGTM, and good call. Any time we can move logic that runs unconditionally at startup to compile time, that's a win.

library/test/src/console.rs Outdated Show resolved Hide resolved
compiler/rustc_builtin_macros/src/test.rs Outdated Show resolved Hide resolved
// pretty-mode:expanded
// pp-exact:tests-are-sorted.pp

extern crate test;
Copy link
Member

Choose a reason for hiding this comment

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

Are we really generating a fresh extern crate test before every #[test]? That seems odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a lot around these changes which seems odd to me.

@ehuss
Copy link
Contributor

ehuss commented Jul 30, 2022

This seems like it will affect all users of libtest, correct? compiletest is one example. I realize that libtest is far from a stable interface, but this could affect other projects.

@saethlin
Copy link
Member Author

Yes, it is possible for other users of libtest to notice. Personally I feel we are well within our rights to change this, because the sorting is not documented anywhere that I can find and this is also behind an unstable feature.

But I do sympathize with users who have come to treat things under #![feature(test)] as de facto stable because the parts they use haven't changed for a long time. Is there some way we could notify maintainers?

@Mark-Simulacrum
Copy link
Member

I think we should try to cover the easily known use cases (e.g., compiletest, compiletest-rs) with patches or PRs, since those affect CI & folks developing in tree immediately.

Beyond that, I think we can also consider whether keeping the sort at runtime is that costly - I think our default sort has "already sorted" detection, maybe that's fast enough to not worry here? It wouldn't be too hard to detect an unsorted input and print a warning (temporarily, we could remove that in ~4 weeks or something).

But I also don't feel we have to do this - I think so long as we fix compiletest{,-rs}, that's okay, and we can also open an issue and pin it so if folks come looking hopefully it's an easy thing to spot. Happy to pin something if you'd like to draft a short note.

@saethlin
Copy link
Member Author

saethlin commented Aug 1, 2022

I think we should try to cover the easily known use cases (e.g., compiletest, compiletest-rs) with patches or PRs

I'm adding a commit for compiletest. compiletest-rs uses tester not the standard library component. tester claims to be a downstream fork of test. Perhaps it is worth letting them know about this situation in general but I think users of tester will not be automatically affected by this PR.

maybe that's fast enough to not worry here?

Unfortunately no. I did think of this initially, but it only brings us down from 4 minutes sorting the tests to 3 minutes. The slowness of Miri is in general super-linear; even if the already-sorted case is a fast path for Rust compiled with LLVM, it's entirely possible that the "fast path" is the slow part of the sorting algorithm under Miri. This is all wobbly and complicated, which is why I'm keen on this change to when we do the sort because it just removes the code path entirely.

@fee1-dead
Copy link
Member

Can we do something with cfg(miri) and turn that part of the code off if miri is run and run a sorted check if it is not miri?

@saethlin
Copy link
Member Author

saethlin commented Aug 2, 2022

We could. But I'm not sure that is a good idea, the whole point of Miri is to check for errors in code, so if we're running different code in Miri and outside of Miri, you could start to wonder what the point of the tool is. And new users are already sometimes suspicious of Miri's claims that their code is UB, I don't want to make understanding it any harder than it already is.

I'm willing to be convinced otherwise, but I think changing semantics of code with a #[cfg(miri)] is something we have deliberately avoided in the past.

@fee1-dead
Copy link
Member

Reassigning this because I am not familiar with the code to approve.

r? rust-lang/compiler

@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 3, 2022
@Mark-Simulacrum
Copy link
Member

I think the next step here is probably to treat this as a change to an unstable libs API and file an ACP (https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html), presuming that geta accepted I'd be comfortable moving ahead here with a pinned issue about this change.

@michaelwoerister
Copy link
Member

@Mark-Simulacrum, do you want to take this?

@saethlin
Copy link
Member Author

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 20, 2022
@thomcc
Copy link
Member

thomcc commented Oct 4, 2022

From libs-api meeting: This is t-libs, because we don't intend to stabilize these APIs and only really have them as public so the compiler can use them.

@rustbot label -T-libs-api +T-libs

I can take the review after work.

r? @thomcc

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 4, 2022
@rustbot rustbot removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 4, 2022
@saethlin
Copy link
Member Author

saethlin commented Oct 8, 2022

@rustbot label -I-libs-api-nominated

@rustbot rustbot removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 8, 2022
@thomcc
Copy link
Member

thomcc commented Oct 8, 2022

Libs side looks fine to me, but it's mostly a compiler change. Can someone review those changes?

r? compiler

@rust-highfive rust-highfive assigned jackh726 and unassigned thomcc Oct 8, 2022
@thomcc thomcc added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2022
@jackh726
Copy link
Member

@bors r=thomcc,jackh726

@bors
Copy link
Contributor

bors commented Oct 23, 2022

📌 Commit df6221a has been approved by thomcc,jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 24, 2022
…ackh726

Sort tests at compile time, not at startup

Recently, another Miri user was trying to run `cargo miri test` on the crate `iced-x86` with `--features=code_asm,mvex`. This configuration has a startup time of ~18 minutes. That's ~18 minutes before any tests even start to run. The fact that this crate has over 26,000 tests and Miri is slow makes a lot of code which is otherwise a bit sloppy but fine into a huge runtime issue.

Sorting the tests when the test harness is created instead of at startup time knocks just under 4 minutes out of those ~18 minutes. I have ways to remove most of the rest of the startup time, but this change requires coordinating changes of both the compiler and libtest, so I'm sending it separately.

(except for doctests, because there is no compile-time harness)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#99578 (Remove redundant lifetime bound from `impl Borrow for Cow`)
 - rust-lang#99939 (Sort tests at compile time, not at startup)
 - rust-lang#102271 (Stabilize `duration_checked_float`)
 - rust-lang#102766 (Don't link to `libresolv` in libstd on Darwin)
 - rust-lang#103277 (Update libstd's libc to 0.2.135 (to make `libstd` no longer pull in `libiconv.dylib` on Darwin))
 - rust-lang#103437 (Sync rustc_codegen_cranelift)
 - rust-lang#103466 (Fix grammar in docs for std::io::Read)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 779418d into rust-lang:master Oct 24, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 30, 2022
Do fewer passes and generally be more efficient when filtering tests

Follow-on of the work I started with this PR: rust-lang#99939

Basically, the startup code for libtest is really inefficient, but that's not usually a problem because it is distributed in release and workloads are small. But under Miri which can be 100x slower than a debug build, these inefficiencies explode.

Most of the diff here is making test filtering single-pass. There are a few other small optimizations as well, but they are more straightforward.

With this PR, the startup time of the `iced` tests with `--features=code_asm,mvex` drops from 17 to 2 minutes (I think Miri has gotten slower under this workload since rust-lang#99939). The easiest way to try this out is to set `MIRI_LIB_SRC` to a checkout of this branch when running `cargo +nightly miri test --features=code_asm,mvex`.

r? `@thomcc`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Nov 5, 2022
Do fewer passes and generally be more efficient when filtering tests

Follow-on of the work I started with this PR: rust-lang/rust#99939

Basically, the startup code for libtest is really inefficient, but that's not usually a problem because it is distributed in release and workloads are small. But under Miri which can be 100x slower than a debug build, these inefficiencies explode.

Most of the diff here is making test filtering single-pass. There are a few other small optimizations as well, but they are more straightforward.

With this PR, the startup time of the `iced` tests with `--features=code_asm,mvex` drops from 17 to 2 minutes (I think Miri has gotten slower under this workload since #99939). The easiest way to try this out is to set `MIRI_LIB_SRC` to a checkout of this branch when running `cargo +nightly miri test --features=code_asm,mvex`.

r? `@thomcc`
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
Do fewer passes and generally be more efficient when filtering tests

Follow-on of the work I started with this PR: rust-lang/rust#99939

Basically, the startup code for libtest is really inefficient, but that's not usually a problem because it is distributed in release and workloads are small. But under Miri which can be 100x slower than a debug build, these inefficiencies explode.

Most of the diff here is making test filtering single-pass. There are a few other small optimizations as well, but they are more straightforward.

With this PR, the startup time of the `iced` tests with `--features=code_asm,mvex` drops from 17 to 2 minutes (I think Miri has gotten slower under this workload since #99939). The easiest way to try this out is to set `MIRI_LIB_SRC` to a checkout of this branch when running `cargo +nightly miri test --features=code_asm,mvex`.

r? `@thomcc`
@saethlin saethlin deleted the pre-sort-tests branch March 15, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-perf Performance improvements that should be mentioned in the release notes. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.