From df6221adc6ad721af59acd501dbfce426b1298da Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 26 Jul 2022 17:40:48 -0400 Subject: [PATCH] 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. --- compiler/rustc_builtin_macros/src/test.rs | 37 ++++++---- .../rustc_builtin_macros/src/test_harness.rs | 15 ++-- compiler/rustc_feature/src/builtin_attrs.rs | 2 +- library/test/src/console.rs | 2 +- library/test/src/lib.rs | 17 ++--- library/test/src/tests.rs | 27 -------- src/librustdoc/doctest.rs | 3 +- src/test/pretty/tests-are-sorted.pp | 69 +++++++++++++++++++ src/test/pretty/tests-are-sorted.rs | 13 ++++ src/tools/compiletest/src/main.rs | 2 + 10 files changed, 130 insertions(+), 57 deletions(-) create mode 100644 src/test/pretty/tests-are-sorted.pp create mode 100644 src/test/pretty/tests-are-sorted.rs diff --git a/compiler/rustc_builtin_macros/src/test.rs b/compiler/rustc_builtin_macros/src/test.rs index 7eee80733666c..92e70bfc779eb 100644 --- a/compiler/rustc_builtin_macros/src/test.rs +++ b/compiler/rustc_builtin_macros/src/test.rs @@ -36,13 +36,22 @@ pub fn expand_test_case( let sp = ecx.with_def_site_ctxt(attr_sp); let mut item = anno_item.expect_item(); item = item.map(|mut item| { + let test_path_symbol = Symbol::intern(&item_path( + // skip the name of the root module + &ecx.current_expansion.module.mod_path[1..], + &item.ident, + )); item.vis = ast::Visibility { span: item.vis.span, kind: ast::VisibilityKind::Public, tokens: None, }; item.ident.span = item.ident.span.with_ctxt(sp.ctxt()); - item.attrs.push(ecx.attribute(ecx.meta_word(sp, sym::rustc_test_marker))); + item.attrs.push(ecx.attribute(attr::mk_name_value_item_str( + Ident::new(sym::rustc_test_marker, sp), + test_path_symbol, + sp, + ))); item }); @@ -215,6 +224,12 @@ pub fn expand_test_or_bench( ) }; + let test_path_symbol = Symbol::intern(&item_path( + // skip the name of the root module + &cx.current_expansion.module.mod_path[1..], + &item.ident, + )); + let mut test_const = cx.item( sp, Ident::new(item.ident.name, sp), @@ -224,9 +239,14 @@ pub fn expand_test_or_bench( Ident::new(sym::cfg, attr_sp), vec![attr::mk_nested_word_item(Ident::new(sym::test, attr_sp))], )), - // #[rustc_test_marker] - cx.attribute(cx.meta_word(attr_sp, sym::rustc_test_marker)), - ], + // #[rustc_test_marker = "test_case_sort_key"] + cx.attribute(attr::mk_name_value_item_str( + Ident::new(sym::rustc_test_marker, attr_sp), + test_path_symbol, + attr_sp, + )), + ] + .into(), // const $ident: test::TestDescAndFn = ast::ItemKind::Const( ast::Defaultness::Final, @@ -250,14 +270,7 @@ pub fn expand_test_or_bench( cx.expr_call( sp, cx.expr_path(test_path("StaticTestName")), - vec![cx.expr_str( - sp, - Symbol::intern(&item_path( - // skip the name of the root module - &cx.current_expansion.module.mod_path[1..], - &item.ident, - )), - )], + vec![cx.expr_str(sp, test_path_symbol)], ), ), // ignore: true | false diff --git a/compiler/rustc_builtin_macros/src/test_harness.rs b/compiler/rustc_builtin_macros/src/test_harness.rs index 89b0d2cc9be2e..a06c2e6419314 100644 --- a/compiler/rustc_builtin_macros/src/test_harness.rs +++ b/compiler/rustc_builtin_macros/src/test_harness.rs @@ -19,9 +19,11 @@ use tracing::debug; use std::{iter, mem}; +#[derive(Clone)] struct Test { span: Span, ident: Ident, + name: Symbol, } struct TestCtxt<'a> { @@ -121,10 +123,10 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> { fn flat_map_item(&mut self, i: P) -> SmallVec<[P; 1]> { let mut item = i.into_inner(); - if is_test_case(&self.cx.ext_cx.sess, &item) { + if let Some(name) = get_test_name(&self.cx.ext_cx.sess, &item) { debug!("this is a test item"); - let test = Test { span: item.span, ident: item.ident }; + let test = Test { span: item.span, ident: item.ident, name }; self.tests.push(test); } @@ -355,9 +357,12 @@ fn mk_tests_slice(cx: &TestCtxt<'_>, sp: Span) -> P { debug!("building test vector from {} tests", cx.test_cases.len()); let ecx = &cx.ext_cx; + let mut tests = cx.test_cases.clone(); + tests.sort_by(|a, b| a.name.as_str().cmp(&b.name.as_str())); + ecx.expr_array_ref( sp, - cx.test_cases + tests .iter() .map(|test| { ecx.expr_addr_of(test.span, ecx.expr_path(ecx.path(test.span, vec![test.ident]))) @@ -366,8 +371,8 @@ fn mk_tests_slice(cx: &TestCtxt<'_>, sp: Span) -> P { ) } -fn is_test_case(sess: &Session, i: &ast::Item) -> bool { - sess.contains_name(&i.attrs, sym::rustc_test_marker) +fn get_test_name(sess: &Session, i: &ast::Item) -> Option { + sess.first_attr_value_str_by_name(&i.attrs, sym::rustc_test_marker) } fn get_test_runner( diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs index 0487270b52a9a..7c063cdec529a 100644 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/compiler/rustc_feature/src/builtin_attrs.rs @@ -727,7 +727,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[ for reserving for `for From for T` impl" ), rustc_attr!( - rustc_test_marker, Normal, template!(Word), WarnFollowing, + rustc_test_marker, Normal, template!(NameValueStr: "name"), WarnFollowing, "the `#[rustc_test_marker]` attribute is used internally to track tests", ), rustc_attr!( diff --git a/library/test/src/console.rs b/library/test/src/console.rs index e9dda98966de3..b1270c272710d 100644 --- a/library/test/src/console.rs +++ b/library/test/src/console.rs @@ -147,7 +147,7 @@ pub fn list_tests_console(opts: &TestOpts, tests: Vec) -> io::Res let mut ntest = 0; let mut nbench = 0; - for test in filter_tests(&opts, tests) { + for test in filter_tests(&opts, tests).into_iter() { use crate::TestFn::*; let TestDescAndFn { desc: TestDesc { name, .. }, testfn } = test; diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs index 3b7193adcc758..e39d383728e8c 100644 --- a/library/test/src/lib.rs +++ b/library/test/src/lib.rs @@ -242,7 +242,7 @@ where let event = TestEvent::TeFiltered(filtered_descs, shuffle_seed); notify_about_test_event(event)?; - let (filtered_tests, filtered_benchs): (Vec<_>, _) = filtered_tests + let (mut filtered_tests, filtered_benchs): (Vec<_>, _) = filtered_tests .into_iter() .enumerate() .map(|(i, e)| (TestId(i), e)) @@ -250,12 +250,12 @@ where let concurrency = opts.test_threads.unwrap_or_else(get_concurrency); - let mut remaining = filtered_tests; if let Some(shuffle_seed) = shuffle_seed { - shuffle_tests(shuffle_seed, &mut remaining); - } else { - remaining.reverse(); + shuffle_tests(shuffle_seed, &mut filtered_tests); } + // Store the tests in a VecDeque so we can efficiently remove the first element to run the + // tests in the order they were passed (unless shuffled). + let mut remaining = VecDeque::from(filtered_tests); let mut pending = 0; let (tx, rx) = channel::(); @@ -295,7 +295,7 @@ where if concurrency == 1 { while !remaining.is_empty() { - let (id, test) = remaining.pop().unwrap(); + let (id, test) = remaining.pop_front().unwrap(); let event = TestEvent::TeWait(test.desc.clone()); notify_about_test_event(event)?; let join_handle = @@ -309,7 +309,7 @@ where } else { while pending > 0 || !remaining.is_empty() { while pending < concurrency && !remaining.is_empty() { - let (id, test) = remaining.pop().unwrap(); + let (id, test) = remaining.pop_front().unwrap(); let timeout = time::get_default_test_timeout(); let desc = test.desc.clone(); @@ -421,9 +421,6 @@ pub fn filter_tests(opts: &TestOpts, tests: Vec) -> Vec {} } - // Sort the tests alphabetically - filtered.sort_by(|t1, t2| t1.desc.name.as_slice().cmp(t2.desc.name.as_slice())); - filtered } diff --git a/library/test/src/tests.rs b/library/test/src/tests.rs index 0b81aff5907e8..79489c40f4e07 100644 --- a/library/test/src/tests.rs +++ b/library/test/src/tests.rs @@ -600,33 +600,6 @@ fn sample_tests() -> Vec { tests } -#[test] -pub fn sort_tests() { - let mut opts = TestOpts::new(); - opts.run_tests = true; - - let tests = sample_tests(); - let filtered = filter_tests(&opts, tests); - - let expected = vec![ - "isize::test_pow".to_string(), - "isize::test_to_str".to_string(), - "sha1::test".to_string(), - "test::do_not_run_ignored_tests".to_string(), - "test::filter_for_ignored_option".to_string(), - "test::first_free_arg_should_be_a_filter".to_string(), - "test::ignored_tests_result_in_ignored".to_string(), - "test::parse_ignored_flag".to_string(), - "test::parse_include_ignored_flag".to_string(), - "test::run_include_ignored_option".to_string(), - "test::sort_tests".to_string(), - ]; - - for (a, b) in expected.iter().zip(filtered) { - assert_eq!(*a, b.desc.name.to_string()); - } -} - #[test] pub fn shuffle_tests() { let mut opts = TestOpts::new(); diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 20ae102bc27d3..6201f3d9ce1f4 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -210,12 +210,13 @@ pub(crate) fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> { pub(crate) fn run_tests( mut test_args: Vec, nocapture: bool, - tests: Vec, + mut tests: Vec, ) { test_args.insert(0, "rustdoctest".to_string()); if nocapture { test_args.push("--nocapture".to_string()); } + tests.sort_by(|a, b| a.desc.name.as_slice().cmp(&b.desc.name.as_slice())); test::test_main(&test_args, tests, None); } diff --git a/src/test/pretty/tests-are-sorted.pp b/src/test/pretty/tests-are-sorted.pp new file mode 100644 index 0000000000000..15dcd4ed97df4 --- /dev/null +++ b/src/test/pretty/tests-are-sorted.pp @@ -0,0 +1,69 @@ +#![feature(prelude_import)] +#![no_std] +#[prelude_import] +use ::std::prelude::rust_2015::*; +#[macro_use] +extern crate std; +// compile-flags: --crate-type=lib --test +// pretty-compare-only +// pretty-mode:expanded +// pp-exact:tests-are-sorted.pp + +extern crate test; +#[cfg(test)] +#[rustc_test_marker = "m_test"] +pub const m_test: test::TestDescAndFn = + test::TestDescAndFn { + desc: test::TestDesc { + name: test::StaticTestName("m_test"), + ignore: false, + ignore_message: ::core::option::Option::None, + compile_fail: false, + no_run: false, + should_panic: test::ShouldPanic::No, + test_type: test::TestType::Unknown, + }, + testfn: test::StaticTestFn(|| test::assert_test_result(m_test())), + }; +fn m_test() {} + +extern crate test; +#[cfg(test)] +#[rustc_test_marker = "z_test"] +pub const z_test: test::TestDescAndFn = + test::TestDescAndFn { + desc: test::TestDesc { + name: test::StaticTestName("z_test"), + ignore: false, + ignore_message: ::core::option::Option::None, + compile_fail: false, + no_run: false, + should_panic: test::ShouldPanic::No, + test_type: test::TestType::Unknown, + }, + testfn: test::StaticTestFn(|| test::assert_test_result(z_test())), + }; +fn z_test() {} + +extern crate test; +#[cfg(test)] +#[rustc_test_marker = "a_test"] +pub const a_test: test::TestDescAndFn = + test::TestDescAndFn { + desc: test::TestDesc { + name: test::StaticTestName("a_test"), + ignore: false, + ignore_message: ::core::option::Option::None, + compile_fail: false, + no_run: false, + should_panic: test::ShouldPanic::No, + test_type: test::TestType::Unknown, + }, + testfn: test::StaticTestFn(|| test::assert_test_result(a_test())), + }; +fn a_test() {} +#[rustc_main] +pub fn main() -> () { + extern crate test; + test::test_main_static(&[&a_test, &m_test, &z_test]) +} diff --git a/src/test/pretty/tests-are-sorted.rs b/src/test/pretty/tests-are-sorted.rs new file mode 100644 index 0000000000000..1f737d5471930 --- /dev/null +++ b/src/test/pretty/tests-are-sorted.rs @@ -0,0 +1,13 @@ +// compile-flags: --crate-type=lib --test +// pretty-compare-only +// pretty-mode:expanded +// pp-exact:tests-are-sorted.pp + +#[test] +fn m_test() {} + +#[test] +fn z_test() {} + +#[test] +fn a_test() {} diff --git a/src/tools/compiletest/src/main.rs b/src/tools/compiletest/src/main.rs index 0e2cc52a645bc..f3f06bde6f166 100644 --- a/src/tools/compiletest/src/main.rs +++ b/src/tools/compiletest/src/main.rs @@ -401,6 +401,8 @@ pub fn run_tests(config: Config) { make_tests(c, &mut tests); } + tests.sort_by(|a, b| a.desc.name.as_slice().cmp(&b.desc.name.as_slice())); + let res = test::run_tests_console(&opts, tests); match res { Ok(true) => {}