Skip to content

Clarify startup #103230

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 20, 2022
Merged

Clarify startup #103230

merged 2 commits into from
Oct 20, 2022

Conversation

nnethercote
Copy link
Contributor

A small follow-up to #102769.

r? @jyn514

- Make the structure of the two variants more similar.
- Add some comments.
- Move various conditional `use` items inside the function that uses
  them.
- Inline some closures.
It took me a while to work this out.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2022
@jyn514
Copy link
Member

jyn514 commented Oct 19, 2022

@bors r+

// The "thread pool" is a single spawned thread in the non-parallel
// compiler. We run on a spawned thread instead of the main thread (a) to
// provide control over the stack size, and (b) to increase similarity with
// the parallel compiler, in particular to ensure there is no accidental
// sharing of data between the main thread and the compilation thread
// (which might cause problems for the parallel compiler).

This hurts performance, right, because we have to start a thread even for very small programs? Does it make sense to avoid using a thread pool for the non-parallel compiler? We can use stacker to run things on an executable heap if we need more stack size; if we want to test that parallel_compiler still works (which imo is not necessary worth the effort) we can run in a separate thread only when debug assertions are enabled.

@bors
Copy link
Collaborator

bors commented Oct 19, 2022

📌 Commit 5d716fd has been approved by jyn514

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 19, 2022
@nnethercote
Copy link
Contributor Author

I haven't measured, but I'd be surprised (horrified) if the cost of spawning one thread is significant even on the smallest programs.

@nnethercote
Copy link
Contributor Author

@bors rollup=always

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#103221 (Fix `SelfVisitor::is_self_ty` ICE)
 - rust-lang#103230 (Clarify startup)
 - rust-lang#103281 (Adjust `transmute{,_copy}` to be clearer about which of `T` and `U` is input vs output)
 - rust-lang#103288 (Fixed docs typo in `library/std/src/time.rs`)
 - rust-lang#103296 (+/- shortcut now only expand/collapse, not both)
 - rust-lang#103297 (fix typo)
 - rust-lang#103313 (Don't label `src/test` tests as `A-testsuite`)
 - rust-lang#103315 (interpret: remove an incorrect assertion)
 - rust-lang#103319 (Improve "`~const` is not allowed here" message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit be4816f into rust-lang:master Oct 20, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 20, 2022
@nnethercote nnethercote deleted the clarify-startup branch October 21, 2022 00:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants