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.

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

Don't require each rustc_interface tool to opt-in to parallel_compiler #113606

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jul 12, 2023

Previously, forgetting to call interface::set_thread_safe_mode would cause the following ICE:

thread 'rustc' panicked at 'uninitialized dyn_thread_safe mode!', /rustc/dfe0683138de0959b6ab6a039b54d9347f6a6355/compiler/rustc_data_structures/src/sync.rs:74:18

This calls set_thread_safe_mode in interface::run_compiler to avoid requiring it in the caller.

Fixes tests/run-make-fulldeps/issue-19371 when parallel-compiler is enabled.

r? @SparrowLii cc #75760

…upport

Previously, forgetting to call `interface::set_thread_safe_mode` would cause the following ICE:
```
thread 'rustc' panicked at 'uninitialized dyn_thread_safe mode!', /rustc/dfe0683138de0959b6ab6a039b54d9347f6a6355/compiler/rustc_data_structures/src/sync.rs:74:18
```

This calls `set_thread_safe_mode` in `interface::run_compiler` to avoid requiring it in the caller.

Fixes `tests/run-make-fulldeps/issue-19371` when parallel-compiler is enabled.
@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2023

Failed to set assignee to SparrowLii: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 12, 2023
@jyn514 jyn514 self-assigned this Jul 12, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jul 12, 2023

I expect this also fixes clippy/miri/rustfmt to support parallel_compiler, or at least makes it more likely they'll work.

@SparrowLii
Copy link
Member

SparrowLii commented Jul 12, 2023

Thanks! It looks good to me.

In the previous discussion in #107586, we think set_dyn_thread_safe_mode should be called as early as possible, since EarlyErrorHandler needs to use Lock. But now we prefer to use safe synchronization structures(Mutex) in Lock when set_dyn_thread_safe_mode is not called(Look #111713). So we just need to ensure that set_dyn_thread_safe_mode is called before Session is created.

This PR transfers set_dyn_thread_safe_mode to be called in run_compiler, which can avoid manually calling set_dyn_thread_safe_mode in rustdoc, rustfmt or other test cases that call run_compiler by themselves.

r? @cjgillot

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 14, 2023

📌 Commit d52eb4f has been approved by cjgillot

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 Jul 14, 2023
@bors
Copy link
Contributor

bors commented Jul 15, 2023

⌛ Testing commit d52eb4f with merge 38aa0038ae0bb9d227a990a0e211069a6176d084...

@bors
Copy link
Contributor

bors commented Jul 15, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2023
@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
curl: (22) The requested URL returned error: 404 

error: failed to download llvm from ci

    help: old builds get deleted after a certain time
    help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:00

@jyn514
Copy link
Member Author

jyn514 commented Jul 15, 2023

@bors retry #113713

@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 Jul 15, 2023
@bors
Copy link
Contributor

bors commented Jul 15, 2023

⌛ Testing commit d52eb4f with merge 4124617...

@bors
Copy link
Contributor

bors commented Jul 16, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 4124617 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 16, 2023
@bors bors merged commit 4124617 into rust-lang:master Jul 16, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 16, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4124617): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.4%, -2.5%] 6
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 659.302s -> 656.574s (-0.41%)

@jyn514 jyn514 deleted the parallel-compiler-cleanup branch July 16, 2023 12:14
@jyn514
Copy link
Member Author

jyn514 commented Jul 16, 2023

this is also noise - see a very similar percentage going the other way on #113514 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants