Clean up rustdoc startup #102769

nnethercote · 2022-10-07T07:30:48Z

Startup is pretty hairy, in both rustdoc and rustc. The first commit here improves the rustdoc situation quite a bit. The remaining commits are smaller but also help.

Best reviewed one commit at a time.

r? @jyn514

jyn514

This is great! Two of the comments I left I found you'd addressed in a later commit :)

In other words, rustdoc calls into rustc_interface at three different
levels. It's a bit confused, and feels like code where functionality has
been added by different people at different times without fully
understanding how the globally accessible stuff is set up.

Yuuuuuup

src/librustdoc/doctest.rs

src/librustdoc/lib.rs

compiler/rustc_interface/src/util.rs

compiler/rustc_interface/src/interface.rs

nnethercote · 2022-10-07T21:01:50Z

@jyn514: I have updated with a new commit that uses spawn_scoped. Thanks for the fast review!

jyn514 · 2022-10-07T22:10:09Z

You're very welcome!

@bors r+

bors · 2022-10-07T22:10:11Z

📌 Commit 1f0463a has been approved by jyn514

It is now in the queue for this repository.

Clean up rustdoc startup Startup is pretty hairy, in both rustdoc and rustc. The first commit here improves the rustdoc situation quite a bit. The remaining commits are smaller but also help. Best reviewed one commit at a time. r? `@jyn514`

Clean up rustdoc startup Startup is pretty hairy, in both rustdoc and rustc. The first commit here improves the rustdoc situation quite a bit. The remaining commits are smaller but also help. Best reviewed one commit at a time. r? ``@jyn514``

Clean up rustdoc startup Startup is pretty hairy, in both rustdoc and rustc. The first commit here improves the rustdoc situation quite a bit. The remaining commits are smaller but also help. Best reviewed one commit at a time. r? ```@jyn514```

Rollup of 7 pull requests Successful merges: - rust-lang#102623 (translation: eager translation) - rust-lang#102769 (Clean up rustdoc startup) - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks) - rust-lang#102847 (impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions) - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`) - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`) - rust-lang#102940 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Clean up rustdoc startup Startup is pretty hairy, in both rustdoc and rustc. The first commit here improves the rustdoc situation quite a bit. The remaining commits are smaller but also help. Best reviewed one commit at a time. r? ````@jyn514````

Dylan-DPC · 2022-10-15T08:39:59Z

failed in rollup but going to try it again individually

@bors rollup=iffy

bors · 2022-10-15T10:17:18Z

⌛ Testing commit 1f0463a with merge 7d6f6a66e3d69e4045adcf7dcd58681175b04450...

bors · 2022-10-15T10:45:30Z

💔 Test failed - checks-actions

nnethercote · 2022-10-17T07:19:00Z

r? @jyn514

bors · 2022-10-18T08:34:38Z

☔ The latest upstream changes (presumably #102992) made this pull request unmergeable. Please resolve the merge conflicts.

rustc's startup has several layers, including: - `interface::run_compiler` passes a closure, `f`, to `run_in_thread_pool_with_globals`, which creates a thread pool, sets up session globals, and passes `f` to `create_compiler_and_run`. - `create_compiler_and_run` creates a `Session`, a `Compiler`, sets the source map, and calls `f`. rustdoc is a bit different. - `main_args` calls `main_options` via `run_in_thread_pool_with_globals`, which (again) creates a thread pool (hardcoded to a single thread!) and sets up session globals. - `main_options` has four different paths. - The second one calls `interface::run_compiler`, which redoes the `run_in_thread_pool_with_globals`! This is bad. - The fourth one calls `interface::create_compiler_and_run`, which is reasonable. - The first and third ones don't do anything of note involving the above functions, except for some symbol interning which requires session globals. In other words, rustdoc calls into `rustc_interface` at three different levels. It's a bit confused, and feels like code where functionality has been added by different people at different times without fully understanding how the globally accessible stuff is set up. This commit tidies things up. It removes the `run_in_thread_pool_with_globals` call in `main_args`, and adjust the four paths in `main_options` as follows. - `markdown::test` calls `test::test_main`, which provides its own parallelism and so doesn't need a thread pool. It had one small use of symbol interning, which required session globals, but the commit removes this. - `doctest::run` already calls `interface::run_compiler`, so it doesn't need further adjustment. - `markdown::render` is simple but needs session globals for interning (which can't easily be removed), so it's now wrapped in `create_session_globals_then`. - The fourth path now uses `interface::run_compiler`, which is equivalent to the old `run_in_thread_pool_with_globals` + `create_compiler_and_run` pairing.

There is no longer any need for them to be separate.

It has a single call site, and removing it slightly improves the confusing tangle of nested closures present at startup.

It has a single call site.

This avoids the need for a degenerate `Lrc::get_mut` call.

It turns out `markdown::render` is more complex than it first appears, because it can invoke `doctest::make_test`, which requires session globals and a thread pool. So this commit changes it to use `interface::run_compiler`. Three of the four paths in `main_args` now use `interface::run_compiler`.

By moving `RenderOptions` out of `Option`, because the two structs' uses are almost entirely separate. The only complication is that `unstable_features` is needed in both structs, but it's a tiny `Copy` type so its duplication seems fine.

jyn514 · 2022-10-18T23:55:24Z

@bors r+

bors · 2022-10-18T23:55:26Z

📌 Commit ca2561a has been approved by jyn514

It is now in the queue for this repository.

bors · 2022-10-19T00:02:43Z

⌛ Testing commit ca2561a with merge 2efc90e...

bors · 2022-10-19T02:49:44Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 2efc90e to master...

rust-timer · 2022-10-19T05:00:16Z

Finished benchmarking commit (2efc90e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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

Cycles

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

the arithmetic mean of the percent change ↩
number of relevant changes ↩

Clarify startup A small follow-up to rust-lang#102769. r? `@jyn514`

eddyb · 2022-11-30T14:38:42Z

compiler/rustc_interface/src/util.rs

-/// Like a `thread::Builder::spawn` followed by a `join()`, but avoids the need
-/// for `'static` bounds.
-#[cfg(not(parallel_compiler))]
-fn scoped_thread<F: FnOnce() -> R + Send, R: Send>(cfg: thread::Builder, f: F) -> R {
-    // SAFETY: join() is called immediately, so any closure captures are still
-    // alive.
-    match unsafe { cfg.spawn_unchecked(f) }.unwrap().join() {
-        Ok(v) => v,
-        Err(e) => panic::resume_unwind(e),
-    }
-}


Please never inline safe functions that contain unsafe code!

(Also, not as important here, but "single call site" is a really bad metric, unless you can find where other callsites were removed in the past or anything of the worst - often single-call-site functions are separate for semantic reasons!)

The purpose of such a function is to encapsulate the unsafety. Inlining could cause unsound interactions with surrounding code in the caller! And increases complexity in analyzing the correctness of the code, regardless of what you may think about some subject readability metric.

In the end this was fine because of the move to the new scoped threads, but this is pretty dangerous to do in the first place, and IMO should've been a PR on its own anyway (that has "scoped threads" or similar in the PR title, assuming going the full route to the new scoped thread API).

eddyb · 2022-11-30T15:01:04Z

compiler/rustc_interface/src/interface.rs

 // JUSTIFICATION: before session exists, only config
 #[allow(rustc::bad_opt_access)]
 pub fn run_compiler<R: Send>(config: Config, f: impl FnOnce(&Compiler) -> R + Send) -> R {
    trace!("run_compiler");
    util::run_in_thread_pool_with_globals(
        config.opts.edition,
        config.opts.unstable_opts.threads,
-        || create_compiler_and_run(config, f),
+        || {


As mentioned in the other comment:

"single call site" is a really bad metric, unless you can find where other callsites were removed in the past or anything of the worst - often single-call-site functions are separate for semantic reasons!

Here it looks like #[allow(rustc::bad_opt_access)] only applied to config.opts.edition, and config.opts.unstable_opts.threads, but now applies to the entire closure.

Not to mention the indentation makes the readability improvement here a bit dubious.

rust-highfive assigned jyn514 Oct 7, 2022

rustbot added 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 Oct 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2022

jyn514 approved these changes Oct 7, 2022

View reviewed changes

src/librustdoc/doctest.rs Show resolved Hide resolved

src/librustdoc/lib.rs Show resolved Hide resolved

compiler/rustc_interface/src/util.rs Outdated Show resolved Hide resolved

compiler/rustc_interface/src/interface.rs Outdated Show resolved Hide resolved

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 7, 2022

Dylan-DPC mentioned this pull request Oct 10, 2022

Rollup of 6 pull requests #102880

Closed

matthiaskrgr mentioned this pull request Oct 11, 2022

Rollup of 8 pull requests #102930

Closed

Dylan-DPC mentioned this pull request Oct 12, 2022

Rollup of 7 pull requests #102960

Closed

Dylan-DPC mentioned this pull request Oct 15, 2022

Rollup of 6 pull requests #103077

Closed

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 Oct 15, 2022

This comment has been minimized.

Sign in to view

nnethercote force-pushed the rustdoc-startup branch from c53d338 to f87fd1f Compare October 17, 2022 01:04

nnethercote added 9 commits October 19, 2022 07:23

Merge main_options into main_args.

2a62c92

There is no longer any need for them to be separate.

Inline and remove scoped_thread.

134e9d3

It has a single call site, and removing it slightly improves the confusing tangle of nested closures present at startup.

Reduce visibility of some functions.

dcc194e

Inline and remove create_compiler_and_run.

b6ae145

It has a single call site.

Apply Lrc later to sess and codegen_backend.

ec409f9

This avoids the need for a degenerate `Lrc::get_mut` call.

Replace a spawn_unchecked with spawn_scoped.

63db9e5

Avoid cloning RenderOptions.

ca2561a

By moving `RenderOptions` out of `Option`, because the two structs' uses are almost entirely separate. The only complication is that `unstable_features` is needed in both structs, but it's a tiny `Copy` type so its duplication seems fine.

nnethercote force-pushed the rustdoc-startup branch from f87fd1f to ca2561a Compare October 18, 2022 22:45

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 18, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 19, 2022

bors merged commit 2efc90e into rust-lang:master Oct 19, 2022

rustbot added this to the 1.66.0 milestone Oct 19, 2022

This was referenced Oct 19, 2022

Resolve documentation links in rustc and store the results in metadata #94857

Merged

errors: generate typed identifiers in each crate #103042

Merged

nnethercote deleted the rustdoc-startup branch October 19, 2022 04:35

nnethercote mentioned this pull request Oct 19, 2022

Clarify startup #103230

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 20, 2022

Rollup merge of rust-lang#103230 - nnethercote:clarify-startup, r=jyn514

be4816f

Clarify startup A small follow-up to rust-lang#102769. r? `@jyn514`

eddyb reviewed Nov 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up rustdoc startup #102769

Clean up rustdoc startup #102769

nnethercote commented Oct 7, 2022

jyn514 left a comment

nnethercote commented Oct 7, 2022

jyn514 commented Oct 7, 2022

bors commented Oct 7, 2022

Dylan-DPC commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

This comment has been minimized.

nnethercote commented Oct 17, 2022

bors commented Oct 18, 2022

jyn514 commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

rust-timer commented Oct 19, 2022

eddyb Nov 30, 2022

eddyb Nov 30, 2022

Clean up rustdoc startup #102769

Clean up rustdoc startup #102769

Conversation

nnethercote commented Oct 7, 2022

jyn514 left a comment

Choose a reason for hiding this comment

nnethercote commented Oct 7, 2022

jyn514 commented Oct 7, 2022

bors commented Oct 7, 2022

Dylan-DPC commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

This comment has been minimized.

nnethercote commented Oct 17, 2022

bors commented Oct 18, 2022

jyn514 commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

rust-timer commented Oct 19, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

eddyb Nov 30, 2022

Choose a reason for hiding this comment

eddyb Nov 30, 2022

Choose a reason for hiding this comment