Use closures more consistently in dep_graph.rs.

[nnethercote]

View all comments

This file has several methods that take a FnOnce() -> R closure:

• DepGraph::with_ignore
• DepGraph::with_query_deserialization
• DepGraph::with_anon_task
• DepGraphData::with_anon_task_inner

It also has two methods that take a faux closure via an A argument and a fn(TyCtxt<'tcx>, A) -> R argument:

• DepGraph::with_task
• DepGraphData::with_task

The rationale is that the faux closure exercises tight control over what state they have access to. This seems silly when (a) they are passed a TyCtxt, and (b) when similar nearby functions take real closures. And they are more awkward to use, e.g. requiring multiple arguments to be gathered into a tuple. This commit changes the faux closures to real closures.

r? @Zalathar

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[nnethercote]

@bors try @rust-timer queue

[Zalathar]

Based on comments, I think there was historically a desire to make sure that dep-graph tasks didn't accidentally pull in other untracked state via closure capture.

But given the existing inconsistency, and the fact that passing tcx gives access to the kitchen sink anyway, there probably isn't any harm in just using normal closures at this point.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 283c322 (283c322d1c3e0fd24ba3187d32027674e634f7e5, parent: b711f95f86b6489b91fdc55c876ed5f95a8d4560)

[rust-timer]

Finished benchmarking commit (283c322): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.5%, -1.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 481.409s -> 481.18s (-0.05%)
Artifact size: 394.91 MiB -> 396.71 MiB (0.45%)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nnethercote]

I have updated this, removing all the parts that changed existing closure type and variable names. I used op: OP for the with_task closures, which (a) matches the existing closures on nearby with_* methods, and (b) are names with more than one letter.