Skip to content

check-aux: test core, alloc, std in Miri #123506

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

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 5, 2024

Let's see if this works, and how long it takes.

@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 5, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2024

Oh...

     Running benches/lib.rs (build/x86_64-unknown-linux-gnu/stage1-std/miri/x86_64-unknown-linux-gnu/debug/deps/allocbenches-a59974b85394f408)

This runs some benchmarks it seems. That will ~never terminate under Miri.^^

@RalfJung RalfJung force-pushed the miri-test-libstd branch 2 times, most recently from 49acb75 to ff3fccd Compare April 5, 2024 19:35
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2024
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 6, 2024

Oh, interesting... the macOS build of compiler-builtins is trying to run a C compiler.

For now let's just run tests on Linux only, then.

@RalfJung RalfJung force-pushed the miri-test-libstd branch 2 times, most recently from 3776b53 to 18955b2 Compare April 6, 2024 08:21
@RalfJung
Copy link
Member Author

RalfJung commented Apr 6, 2024

Strangely the macos tests work fine locally. Not sure what is different on CI...

@RalfJung
Copy link
Member Author

RalfJung commented Apr 6, 2024

All right, tests are passing!

  • core, alloc lib tests: 20 minutes
  • core, alloc doc tests: 20 minutes
  • std lib tests: 3 minutes
  • std doc tests: 1 minute

On std we're unfortunately only testing one target as somehow the compiler-builtins build script fails for other targets. I'll extend the std tests a bit (we need some filters here as not all of std is supported by Miri).

There are likely some testcases that are unproportionally slow. They are somewhat hard to identify. For doctests we get the "has been running for a long time" warning, which points at this test that creates a 100k element linked list. For libtests, the test harness runs inside Miri where it disables timing in Miri to make sure it can run with isolation enabled. Now that we have the fake clock in isolation mode, maybe we can fix that. Whether that gives useful results is a different question...

/// for i in 0..100000 {
/// let size = 100000;
/// # let size = if cfg!(miri) { 100 } else { size };
/// for i in 0..size {
Copy link
Member Author

Choose a reason for hiding this comment

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

Is that an okay approach to make the test shorter in Miri? The alternative would be more convoluted but would let the code look as before on the web view:

# let size = if cfg!(miri) { 100 } else { 100000 };
# for i in 0..size {
# /*
for i in 0..100000 {
# */

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to me, especially given it's relatively isolated. Realistically I'm not convinced this example needs a huge length (100 elements or 100,000 is about equally bad if you're pushing a new stack frame per element, IMO, given that you might already be close to the end of the allowed stack when starting to execute).

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 6, 2024

It's actually a bit surprising that these take so long, since the out-of-tree test tests alloc and core in 45 minutes total and it runs all tests on 2 targets. So the test here should have been about twice as fast.

Do we have debug assertions enabled on the gnu-aux runner, or something like that?

@RalfJung
Copy link
Member Author

RalfJung commented Apr 6, 2024

Cc @Mark-Simulacrum

@ChrisDenton
Copy link
Member

Oh, interesting... the macOS build of compiler-builtins is trying to run a C compiler.

presumably the build.optimized-compiler-builtins config is set to true.

@Mark-Simulacrum
Copy link
Member

Oh, interesting... the macOS build of compiler-builtins is trying to run a C compiler.

presumably the build.optimized-compiler-builtins config is set to true.

Yes:

RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.optimized-compiler-builtins"

Do we have debug assertions enabled on the gnu-aux runner, or something like that?

Yes:

rust/src/ci/run.sh

Lines 125 to 140 in 01f7f3a

# We almost always want debug assertions enabled, but sometimes this takes too
# long for too little benefit, so we just turn them off.
if [ "$NO_DEBUG_ASSERTIONS" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-debug-assertions"
fi
# Same for overflow checks
if [ "$NO_OVERFLOW_CHECKS" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-overflow-checks"
fi
# In general we always want to run tests with LLVM assertions enabled, but not
# all platforms currently support that, so we have an option to disable.
if [ "$NO_LLVM_ASSERTIONS" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-assertions"
fi

@RalfJung RalfJung marked this pull request as ready for review April 7, 2024 08:00
# In `std` we cannot test everything.
$(Q)MIRIFLAGS="-Zmiri-disable-isolation" BOOTSTRAP_SKIP_TARGET_SANITY=1 \
$(BOOTSTRAP) miri --stage 2 library/std \
--no-doc -- \
Copy link
Member Author

@RalfJung RalfJung Apr 7, 2024

Choose a reason for hiding this comment

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

I didn't add mips-unknown-linux-gnu here as I think std has much less code that depends on bitwidth or endianess, so the extra 10min CI time does not seem worth it. Instead we have the macOS and Windows checks below, as std does contain a lot of OS-specific code. (And it's a 32bit Windows so at least that kind of architecture is covered.)

@RalfJung
Copy link
Member Author

RalfJung commented Apr 7, 2024

Ah, that's more like it. Now even when testing two targets for core and alloc we get

  • core+alloc libtests: 30min
  • core+alloc doctests: 10min
  • std libtests: 10min
  • std doctests: 1min
  • std reduced libtests for extra targets: 11min

The total duration of the gnu-aux job is around 104min.

That is just 2min above the anticipated 1h. I can remove some of the extra targets if that is desired. Testing core and alloc only on one target brings it down by about 15min.

r? @Mark-Simulacrum

@rustbot rustbot assigned Mark-Simulacrum and unassigned onur-ozkan Apr 7, 2024
@RalfJung RalfJung changed the title check-aux: test core and alloc in Miri check-aux: test core, alloc, std in Miri Apr 7, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Apr 7, 2024

Some data from #123560: enabling just overflow checks add around 10min to total CI time, enabling just debug assertions adds around 30min.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 7, 2024

@rustbot ready

@rustbot rustbot 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 Apr 7, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+

I think this is good to land. We can always tweak it further in future PRs (e.g., get rid of the Makefile, maybe try to enable benchmarks running once or something like that).

@bors
Copy link
Collaborator

bors commented Apr 7, 2024

📌 Commit 596908b has been approved by Mark-Simulacrum

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 Apr 7, 2024
@bors
Copy link
Collaborator

bors commented Apr 8, 2024

⌛ Testing commit 596908b with merge a2c72ce...

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing a2c72ce to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2024
@bors bors merged commit a2c72ce into rust-lang:master Apr 8, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 8, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a2c72ce): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.5% [0.4%, 0.8%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [1.0%, 1.4%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [1.0%, 1.4%] 4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.322s -> 668.882s (0.08%)
Artifact size: 318.48 MiB -> 318.31 MiB (-0.05%)

@RalfJung
Copy link
Member Author

RalfJung commented Apr 8, 2024 via email

@Kobzol
Copy link
Contributor

Kobzol commented Apr 8, 2024

This PR had nothing to do with our benchmarking infrastructure :) So this has to be pure noise.

@RalfJung RalfJung deleted the miri-test-libstd branch April 8, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants