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.

Sign up for GitHub

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

Jump to bottom

libtest: run all tests in their own thread, if supported by the host #103681

Merged
merged 1 commit into from
Nov 4, 2022
Merged

libtest: run all tests in their own thread, if supported by the host #103681

merged 1 commit into from
Nov 4, 2022
+42 −59

Conversation

RalfJung
Copy link
Member

This reverts the threading changes of #56243, which made it so that with -j1, the test harness does not spawn any threads. Those changes were done to enable Miri to run the test harness, but Miri supports threads nowadays, so this is no longer needed. Using a thread for each test is useful because the thread's name can be set to the test's name which makes panic messages consistent between -j1 and -j2 runs and also a bit more readable.

I did not revert the HashMap changes of #56243; using a deterministic map seems fine for the test harness and the more deterministic testing is the better.

Fixes #59122
Fixes #70492

@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2022

r? @joshtriplett

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 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

@RalfJung
Copy link
Member Author

I'll need to do some testing to ensure the test harness still works on Windows, which has less complete threading support in Miri than Unix targets.

@rust-log-analyzer

This comment was marked as resolved.

Sign in to view
@RalfJung RalfJung mentioned this pull request Oct 28, 2022
Closed
4 tasks
@RalfJung
Copy link
Member Author

RalfJung commented Oct 28, 2022 •
edited

Unfortunately on the Miri side of this is blocked on rust-lang/miri#2628.

@RalfJung RalfJung force-pushed the libtest-thread branch 2 times, most recently from 7e039e0 to f7f381e Compare October 28, 2022 10:04
@the8472
Copy link
Member

the8472 commented Oct 28, 2022

Using a thread for each test is useful because the thread's name can be set to the test's name

On many-core systems it could be useful to use a thread pool rather than a thread-per-test to avoid the thread-spawning costs (e.g. contention on the memory-map lock for the thread stack creation). Maybe updating the thread name between tests could work?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 28, 2022 •
edited

On many-core systems it could be useful to use a thread pool rather than a thread-per-test to avoid the thread-spawning costs (e.g. contention on the memory-map lock for the thread stack creation).

Sure, but that's a new feature I won't implement. All this here does is revert a PR I wrote years ago. It doesn't even change behavior for many-core systems, it only changes behavior for -j1.

Maybe updating the thread name between tests could work?

The standard library doesn't have an API for that. Also see #70492.

@RalfJung RalfJung marked this pull request as draft October 28, 2022 11:48
@RalfJung
Copy link
Member Author

rust-lang/miri#2630 adds the missing thread parker support for Windows. :)

@RalfJung RalfJung marked this pull request as ready for review October 29, 2022 10:38
@RalfJung
Copy link
Member Author

All right, with #103721 on its way this is ready for review (but shouldn't land before that Miri update lands).
r? @thomcc

@rustbot rustbot assigned thomcc and unassigned joshtriplett Oct 29, 2022
thomcc
thomcc approved these changes Oct 29, 2022
View reviewed changes
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Huh, I had no idea the reason miri was the reason for this part of libtest's cruftiness.

Really glad to see it gone either way, it vaguely was in the way of a few other things I've wanted to do for a while now.

r=me once you're ready to land it.

@RalfJung
Copy link
Member Author

Thanks! #103721 has landed, so this is good to go.

@bors r=thomcc

@bors
Copy link
Contributor

bors commented Oct 30, 2022

📌 Commit 3af058e has been approved by thomcc

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 30, 2022
@bjorn3
Copy link
Member

bjorn3 commented Oct 30, 2022

What about inherently single threaded environments like wasm? Won't this break libtest for those? As far as I am aware wasm32-wasi currently supports using libtest.

@RalfJung
Copy link
Member Author

@bjorn3 the "if supported by the host" part should take care of that. Ctrl-F supports_threads in the code.

@bjorn3
Copy link
Member

bjorn3 commented Oct 30, 2022

Missed that, my bad.

@matthiaskrgr matthiaskrgr mentioned this pull request Nov 4, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2022
@bors
Auto merge of rust-lang#103978 - matthiaskrgr:rollup-iym9kmg, r=matth…
0950848 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#103367 (Remove std's transitive dependency on cfg-if 0.1)
 - rust-lang#103397 (Port `dead_code` lints to be translatable.)
 - rust-lang#103681 (libtest: run all tests in their own thread, if supported by the host)
 - rust-lang#103792 (Migrate `codegen_ssa` to diagnostics structs - [Part 2])
 - rust-lang#103897 (asm: Work around LLVM bug on AArch64)
 - rust-lang#103937 (minor changes to make method lookup diagnostic code easier to read)
 - rust-lang#103958 (Test tidy should not count untracked paths towards entries limit)
 - rust-lang#103964 (Give a specific lint for unsafety not being inherited)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c38ee06 into rust-lang:master Nov 4, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 4, 2022
@RalfJung RalfJung deleted the libtest-thread branch November 5, 2022 10:24
@Taaitaaiger Taaitaaiger mentioned this pull request Nov 6, 2022
Open
@ianks ianks mentioned this pull request Jan 28, 2023
Closed
@ehuss ehuss mentioned this pull request Mar 27, 2023
Closed
@knopp
Copy link

knopp commented Jul 27, 2023

Rust default test harness doesn't really work for anything on macOS that requires execution on main thread. Unfortunately main thread is special (so that on some places there are actually checks for pthread_main_np, i.e. in libdispatch. It would be immensely helpful if there was way to run tests with the default harness directly on main thread.

@RalfJung
Copy link
Member Author

@knopp comments on old merged MRs are unlikely to lead anywhere. Please open a new issue to describe the problem.

@RalfJung
Copy link
Member Author

See e.g. #104053

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomcc thomcc thomcc approved these changes

Assignees

@thomcc thomcc

Labels
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
Milestone
1.67.0
9 participants
@RalfJung @rustbot @rust-log-analyzer @the8472 @bors @bjorn3 @knopp @thomcc @joshtriplett