Add tests for #58319 and #65131 #70176
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @Centril |
|
Thanks! @bors r+ |
|
📌 Commit 5444ade has been approved by |
bors
added
S-waiting-on-bors
Add tests for rust-lang#58319 and rust-lang#65131 Fixes rust-lang#58319 and fixes rust-lang#65131
Rollup of 16 pull requests Successful merges: - rust-lang#65097 (Make std::sync::Arc compatible with ThreadSanitizer) - rust-lang#69033 (Use generator resume arguments in the async/await lowering) - rust-lang#69997 (add `Option::{zip,zip_with}` methods under "option_zip" gate) - rust-lang#70038 (Remove the call that makes miri fail) - rust-lang#70058 (can_begin_literal_maybe_minus: `true` on `"-"? lit` NTs.) - rust-lang#70111 (BTreeMap: remove shared root) - rust-lang#70139 (add delay_span_bug to TransmuteSizeDiff, just to be sure) - rust-lang#70165 (Remove the erase regions MIR transform) - rust-lang#70166 (Derive PartialEq, Eq and Hash for RangeInclusive) - rust-lang#70176 (Add tests for rust-lang#58319 and rust-lang#65131) - rust-lang#70177 (Fix oudated comment for NamedRegionMap) - rust-lang#70184 (expand_include: set `.directory` to dir of included file.) - rust-lang#70187 (more clippy fixes) - rust-lang#70188 (Clean up E0439 explanation) - rust-lang#70189 (Abi::is_signed: assert that we are a Scalar) - rust-lang#70194 (#[must_use] on split_off()) Failed merges: r? @ghost
|
Oh; it was just 25s vs. 1s? Well that wouldn't make any difference in the testsuite in CI terms, but I guess folks might notice locally. cc @Mark-Simulacrum in case they know how to ensure a test doesn't run too long; if not, perhaps we should add support to compiletest? |
|
If we want to compare performance, the test should go onto perf.rlo, not into UI tests; UI tests are not intended for performance comparisons today. If we just want to make sure the test doesn't take too long (and 25 seconds is pretty long -- are we sure we need such a long test? Though it sounds like maybe it's shorter now), then I just wouldn't worry about it, if it finishes relatively quickly locally then that's good enough. I would recommend trying to make sure tests finish in less than 1 second. |
|
@Mark-Simulacrum This is intended to be a regression test for a specific performance fix that reduced the asymptotic compile time on the test snippet from While in general perf.rlo is the correct place for benchmarks, I'm not sure whether this test is representative enough to make much sense there (as in "is it worth being a plot on the perf.rlo front page"). The idea is just to have a test that the asymptotic performance does not accidentally become much worse. The situation under test is a corner case (reporting borrow errors in huge functions) that normally does not come up unless you're debugging code generation (and your generator makes huge functions), so such a regression could go unnoticed. |
|
If it's short (or can be made to compile quickly, roughly in less than 2 seconds, after the fix) we can add it to perf, I wouldn't worry too much about "representative" in that sense. That certainly sounds like a test that should be on perf and wouldn't make much sense as a test in this repository. |
|
@Mark-Simulacrum the test compiles very quickly now. I can move it to perf. |
Fixes #58319 and fixes #65131