Load the dep graph before parsing #115693

Zoxc · 2023-09-09T03:58:57Z

This changes the dep graph to start loading before parsing. We don't have access to the crate name defined in the source code at this point, so if it's not specified on the command line, incremental compilation will fallback to the file path as a crate identifier.

r? @oli-obk

bjorn3 · 2023-09-09T10:27:58Z

compiler/rustc_incremental/src/persist/fs.rs

+    // Create a hash for metadata and the rust version
+    let metadata_and_version = StableCrateId::new(
+        symbol::kw::Empty,
+        false,


Always specifying false here will cause the incr comp cache for libs and bins with the same crate name to conflict. This argument exists to avoid this conflict.

I guess we can support that if the crate types are specified on the command line.

oli-obk · 2023-09-09T12:43:24Z

@bors try @rust-timer queue

bors · 2023-09-09T12:44:21Z

⌛ Trying commit 360ee87 with merge bcdfb0b...

Load the dep graph before parsing This changes the dep graph to start loading before parsing. We don't have access to the crate name defined in the source code at this point, so if it's not specified on the command line, incremental compilation will fallback to the file path as a crate identifier. r? `@oli-obk`

bors · 2023-09-09T13:55:38Z

☀️ Try build successful - checks-actions
Build commit: bcdfb0b (bcdfb0ba73fd4d7c293ea14264483584b49f3b86)

rust-timer · 2023-09-09T15:29:57Z

Finished benchmarking commit (bcdfb0b): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	2.5%	[1.6%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-4.2%, -1.0%]	3
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

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.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 627.323s -> 628.994s (0.27%)
Artifact size: 318.09 MiB -> 318.03 MiB (-0.02%)

oli-obk

why are we using the crate name as part of the stable crate id at all if there are workarounds that don't need them? Or similarly for crate_types

oli-obk · 2023-09-11T12:47:08Z

compiler/rustc_incremental/src/persist/fs.rs

+    // Use the crate name from the command line if available and valid
+    let crate_name = sess.opts.crate_name.as_ref().and_then(|name| {
+        name.as_str().chars().all(|c| c.is_alphanumeric() || c == '_').then_some(&name[..])
+    });


does this duplicate some logic from somewhere?

We should probably ensure they stay in sync (ideally by deduplicating, maybe via a Sess method?)

oli-obk · 2023-09-11T12:54:23Z

I'm not really sure about the possible effects this PR can have.

I guess the core part is that the stable crate id is entirely independent of the crate source.

This should probably go through an MCP as there has gone a lot of discussion and design into the current behaviour, and we should not just change without larger feedback.

I think something in this direction is ok, but I also think it needs a larger refactoring of StableCrateId::new

Zoxc · 2023-09-16T18:20:59Z

The primary negative effect would be reusing incremental artifacts from the wrong crate which slows down compilation due to them having to be copied on disk, load to memory and discarded.

I'm not a huge fan of including file names as crate identity here. There could be some issues with shifting paths (shared folders, renaming, etc.). My preferred semantics would be having -C incremental strictly refer to the current crate with no concurrent sessions allowed. That allows discarding of all stale files and rustc would also not need to copy as many files around. That's not compatible with Cargo's current usage however. I have been thinking about a --incremental flag to switch to that behavior.

I don't know how -C incremental is used outside of Cargo. The documentation for -C incremental basically just says it enables incremental compilation, so I wouldn't expect that to constrain usage much.

apiraino · 2023-10-05T13:25:09Z

I'll take @oli-obk suggestion in this comment and label this PR as needing an MCP. Please @Zoxc feel free to provide more context (I may be missing some bits), thanks

@rustbot author

Zoxc · 2024-02-28T22:57:43Z

Closing in favor of #121764.

rustbot assigned oli-obk Sep 9, 2023

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. labels Sep 9, 2023

bjorn3 reviewed Sep 9, 2023

View reviewed changes

Zoxc force-pushed the early-dep-graph-load branch from bf9c899 to 8ca0263 Compare September 9, 2023 11:49

This comment has been minimized.

Sign in to view

Load the dep graph before parsing

360ee87

Zoxc force-pushed the early-dep-graph-load branch from 8ca0263 to 360ee87 Compare September 9, 2023 12:14

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2023

oli-obk reviewed Sep 11, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2023

Dylan-DPC added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Nov 11, 2023

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2024

Zoxc mentioned this pull request Feb 26, 2024

Separate incremental crate identity rust-lang/compiler-team#726

Closed

3 tasks

Zoxc closed this Feb 28, 2024

apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 29, 2024

apiraino removed the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load the dep graph before parsing #115693

Load the dep graph before parsing #115693

Zoxc commented Sep 9, 2023

bjorn3 Sep 9, 2023

Zoxc Sep 9, 2023

This comment has been minimized.

oli-obk commented Sep 9, 2023

This comment has been minimized.

bors commented Sep 9, 2023

bors commented Sep 9, 2023

This comment has been minimized.

rust-timer commented Sep 9, 2023

oli-obk left a comment

oli-obk Sep 11, 2023

oli-obk commented Sep 11, 2023

Zoxc commented Sep 16, 2023

apiraino commented Oct 5, 2023

Zoxc commented Feb 28, 2024

Load the dep graph before parsing #115693

Load the dep graph before parsing #115693

Conversation

Zoxc commented Sep 9, 2023

bjorn3 Sep 9, 2023

Choose a reason for hiding this comment

Zoxc Sep 9, 2023

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk commented Sep 9, 2023

This comment has been minimized.

bors commented Sep 9, 2023

bors commented Sep 9, 2023

This comment has been minimized.

rust-timer commented Sep 9, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Sep 11, 2023

Choose a reason for hiding this comment

oli-obk commented Sep 11, 2023

Zoxc commented Sep 16, 2023

apiraino commented Oct 5, 2023

Zoxc commented Feb 28, 2024