Add parallel front end robustness test to ui tests #132051
Open
+654
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
rustbot
added
A-compiletest
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
SparrowLii
added
the
WG-compiler-parallel
|
r? jieyouxu |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jieyouxu
requested changes
Oct 23, 2024
There was a problem hiding this comment.
Thanks for the PR, this seems like a good improvement in test coverage for parallel front-end.
Could you please split the changes into at least two more-or-less atomic commits? I.e.
- Commit 1 implements compiletest changes.
- Commit 2 updates the tests.
As for the changes themselves, I noticed:
- Many tests don't have much context (no explanation about what they're trying to check, no brief summary, no backlinks to issues). Are these just basic smoke tests for parallel frontend? If it seems overkill to add such comments to the individual tests, could you leave a basic SUMMARY.md inside
tests/ui/parallel-frontendto summarize the categories of tests and what issues they're intending to check? This is so that if anyone ever tries to debug why the tests failed/passed, they can have some clues as guidance. The summaries don't have to be super detailed, but even a brief sentence for test intention can go a long way.
Please also refer to individual review comments. If you have any questions about bootstrap/compiletest/general test infra, I might be able to help.
Comment on lines
+33
to
+37
| // Repeated testing due to instability in multithreaded environments. | ||
| for _ in 0..50 { | ||
| proc_res = self.compile_test(should_run, emit_metadata); | ||
| self.check_no_compiler_crash(&proc_res, false); | ||
| } |
There was a problem hiding this comment.
Question: this is going to be expensive as running 50 times involves not only spawning multiple processes, but also more expensive is the compiling itself. Do we have any local benchmarks to see how much longer the test will take w/ the extra 49 runs compared to just running once? I'm not suggesting we shouldn't do this -- we should -- but I would like to have more info on the impact of this on test time and then make an informed decision to accept the cost.
Context: I'm primarily thinking about some tests that might take a long time to compile, e.g. tests that have deep recursive types.
There was a problem hiding this comment.
I haven't found a good benchmark yet, from what I've observed so far these tests don't noticeably extend the test time though. I think we can keep this comment until we get more information later
tests/ui/parallel-rustc/cache-after-waiting-issue-111528.stderr
Outdated
Show resolved
Hide resolved
rustbot
added
S-waiting-on-author
|
Thanks for the reviewing! I gonna make the changes soon |
jieyouxu
added
the
A-test-infra
SparrowLii
force-pushed
the
parallel_ui_test
branch
from
c6c447c
to
1a125e5
Compare
|
Just resubmitted the commits. Since these tests are used to test ice problems such as deadlock, we don't actually care about the error messages output. I deleted all error output to simplify the function and review |
SparrowLii
changed the title
|
The job Click to see the possible cause of the failure (guessed by this bot) |
jieyouxu
reviewed
Oct 26, 2024
Comment on lines
-1352
to
-1354
| // Use a single thread for efficiency and a deterministic error message order | ||
| rustc.arg("-Zthreads=1"); | ||
|
|
There was a problem hiding this comment.
Question: though if we no longer have the -Z threads=1, would other non-parallel tests potentially become flaky? Or is the parallel front-end currently such that by default is only -Z threads=1? Or rather, is that an intended feature (that we may discover some ui tests being flaky and thus expose bugs in current parallel rustc infra?)
There was a problem hiding this comment.
The thread of parallel front end is always 1 only if users pass -Zthreads=n mannually. On the other hand, the user can easily break the limit by passing the -Z threads option(Later option will reset the previous one, we may need to improve on this), so this code doesn't make much sense.
Defaulting the number of threads to be greater than 1 is intended but will not be implemented for a short time. When we do, I think we still don't need to manually limit -Z threads=1 in test suit, but change the it to fit the output of a multithreaded environment.
jieyouxu
reviewed
Oct 26, 2024
| //@ compile-flags: -Z threads=50 | ||
|
|
||
| #![feature(generic_const_exprs)] | ||
| //~^ WARNING |
|
Also, we may want to open a #t-compiler thread (and maybe in libs/bootstrap) for awareness (esp. for reviewers) about parallel rustc tests and what to do if they ever encounter a related ICE. |
|
☔ The latest upstream changes (presumably #132317) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
update #118698
Most of the issues of parallel front end are not reproducible, and are even fixed in compiler updates (#124423 is no longer reproducible after I updated the master branch)
This PR add all testable issues to UI tests to verify their irreproducibility. If no problems occur, and no feedback from other CI testing was received within a certain period of time, we can close the source PR