Better debug logs for borrowck constraint graph #104239

b-naber · 2022-11-10T11:13:38Z

It's really cumbersome to work with RegionVars when trying to debug borrowck code or when trying to understand how the borrowchecker works. This PR collects some region information (behind cfg(debug_assertions)) for created RegionVars (NLL region vars, this PR doesn't touch canonicalization) and prints the nodes and edges of the strongly connected constraints graph using representatives that use that region information (either lifetime names, locations in MIR or spans).

rustbot · 2022-11-10T11:13:43Z

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

Noratrieb · 2022-11-10T12:46:09Z

This adds a huge amount of #[cfg]ed code, which isn't very good. I think most of this can be replaced with cfg!(debug_assertions) or other control-flow based conditional execution instead, which is way easier to maintain.
With something like

fn with_reg_var_to_origin(&self, f: impl FnOnce(RefCell<FxHashMap<ty::RegionVid, RegionCtxt>>))

that calls the closure when debug_assertions are enabled and is a noop when they aren't, the #[cfg] attributes could be contained to a minimum (one for the field, one for new and one for something like the function above).

b-naber · 2022-11-10T13:18:53Z

This adds a huge amount of #[cfg]ed code, which isn't very good. I think most of this can be replaced with cfg!(debug_assertions) or other control-flow based conditional execution instead, which is way easier to maintain. With something like
fn with_reg_var_to_origin(&self, f: impl FnOnce(RefCell<FxHashMap<ty::RegionVid, RegionCtxt>>))
that calls the closure when debug_assertions are enabled and is a noop when they aren't, the #[cfg] attributes could be contained to a minimum (one for the field, one for new and one for something like the function above).

Something like this would have the downside of passing in unused arguments (the RegionCtxt) in certain functions when debug_assertions if off though (if I understand your approach correctly). If not, can you please elaborate on your approach?

bors · 2022-11-11T23:22:10Z

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

jackh726 · 2022-12-01T18:34:31Z

Can we try to add this without all the cfgs? And measure perf? Then add as few cfgs as possible to not regress perf?

b-naber · 2022-12-14T20:40:19Z

Can we try to add this without all the cfgs? And measure perf? Then add as few cfgs as possible to not regress perf?

@jackh726 Sorry, didn't immediately have time when I saw your reply and then forgot that you posted.

bors · 2022-12-23T03:24:15Z

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

jackh726 · 2022-12-26T20:17:59Z

Agh sorry for the delay. Can you rebase and remove the cargo.lock changes?

jackh726 · 2023-01-18T13:09:47Z

@bors try @rust-timer queue

jackh726 · 2023-01-18T13:09:59Z

Agh

jackh726 · 2023-01-18T13:10:15Z

@bors try

jackh726 · 2023-02-20T04:45:08Z

Okay good. Can we do one more with the cfgs removed?

b-naber · 2023-02-20T22:02:07Z

Forgot that I also should be able to request a perf run, let's see:

@bors try @rust-timer queue

bors · 2023-02-20T22:02:20Z

⌛ Trying commit c9843d6 with merge e02ff57926ffbe7e5ed3694d530b665c6917be1d...

bors · 2023-02-21T00:13:55Z

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

rust-timer · 2023-02-21T01:19:52Z

Finished benchmarking commit (e02ff57926ffbe7e5ed3694d530b665c6917be1d): 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)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

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)	2.6%	[2.2%, 3.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

jackh726 · 2023-02-21T12:56:37Z

Okay, unless there's anything else you think you want to try, I think we land this as-is. How's that sound @b-naber?

b-naber · 2023-02-21T16:21:46Z

@jackh726 Don't have anything else to try.

jackh726

Just a couple nits. r=me with or without them

jackh726 · 2023-02-21T16:57:52Z

compiler/rustc_data_structures/src/graph/scc/mod.rs

-    scc_indices: IndexVec<N, S>,
+    pub scc_indices: IndexVec<N, S>,

    /// Data about each SCC.
-    scc_data: SccData<S>,
+    pub scc_data: SccData<S>,
 }

-struct SccData<S: Idx> {
+pub struct SccData<S: Idx> {
    /// For each SCC, the range of `all_successors` where its
    /// successors can be found.
-    ranges: IndexVec<S, Range<usize>>,
+    pub ranges: IndexVec<S, Range<usize>>,

    /// Contains the successors for all the Sccs, concatenated. The
    /// range of indices corresponding to a given SCC is found in its
    /// SccData.
-    all_successors: Vec<S>,
+    pub all_successors: Vec<S>,


I kind of think these should stay private, with public functions that return immutable access?

jackh726 · 2023-02-21T16:59:50Z

compiler/rustc_middle/src/ty/sty.rs

    pub fn is_var(self) -> bool {
        matches!(self.kind(), ty::ReVar(_))
    }
+
+    pub fn try_get_var(self) -> Option<RegionVid> {
+        match self.kind() {
+            ty::ReVar(vid) => Some(vid),
+            _ => None,
+        }
+    }


just a nit, but I kind of like as_var or just var more.

Also, in theory, is_var is redundant now, because you can just do l.as_var().is_some()

jackh726 · 2023-02-21T17:00:41Z

compiler/rustc_borrowck/src/renumber.rs

 {
    infcx.tcx.fold_regions(value, |_region, _depth| {
        let origin = NllRegionVariableOrigin::Existential { from_forall: false };
-        infcx.next_nll_region_var(origin)
+        infcx.next_nll_region_var(origin, || get_ctxt_fn())


Can just pass get_ctxt_fn right?

No, get_ctxt_fn is captured in fold_regions which is FnMut, so we can't move there.

b-naber · 2023-02-21T21:58:29Z

@bors r=jackh726 rollup

bors · 2023-02-21T21:58:31Z

📌 Commit 8252a6e has been approved by jackh726

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#104239 (Better debug logs for borrowck constraint graph) - rust-lang#108202 (Make sure `test_type_match` doesn't ICE with late-bound types) - rust-lang#108295 (Use DefKind to give more item kind information during BindingObligation note ) - rust-lang#108306 (compiletest: up deps) - rust-lang#108313 (Fix compiletest possible crash in option only-modified) - rust-lang#108322 (Clean ConstProp) - rust-lang#108323 (hir-analysis: make one diagnostic translatable) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

There's a bad pattern matching confusion present in this function. `_anon` gets assigned to, and then `_anon` is used as an unbound variable in the pattern, which is unrelated to the first `_anon`. If the `_anon` didn't start with `_` the compiler would give warnings. This was introduced in rust-lang#104239. I have rewritten the function to remove the confusion and preserve the existing behaviour. This seems safest, because the original intent is not clear.

rustbot assigned jackh726 Nov 10, 2022

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

This comment has been minimized.

Sign in to view

b-naber force-pushed the sccs-info branch from fabea6e to acea19c Compare November 10, 2022 13:23

b-naber force-pushed the sccs-info branch from acea19c to a179d1b Compare December 14, 2022 20:37

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Dec 14, 2022

b-naber force-pushed the sccs-info branch from a179d1b to e794e12 Compare December 14, 2022 20:38

b-naber force-pushed the sccs-info branch from e794e12 to 269f1e0 Compare December 14, 2022 20:53

jackh726 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 Dec 26, 2022

b-naber force-pushed the sccs-info branch 2 times, most recently from 8dc0366 to 570ad62 Compare January 5, 2023 11:31

This comment was marked as off-topic.

Sign in to view

rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023

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 Jan 18, 2023

jackh726 closed this Jan 18, 2023

jackh726 reopened this Jan 18, 2023

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Feb 20, 2023

remove cfg attributes

c9843d6

rust-cloud-vms bot force-pushed the sccs-info branch from 0e917b4 to c9843d6 Compare February 20, 2023 22:00

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 Feb 20, 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 Feb 21, 2023

jackh726 approved these changes Feb 21, 2023

View reviewed changes

address review

8252a6e

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2023

matthiaskrgr mentioned this pull request Feb 21, 2023

Rollup of 7 pull requests #108325

Merged

bors merged commit 314fe4d into rust-lang:master Feb 22, 2023

rustbot added this to the 1.69.0 milestone Feb 22, 2023

b-naber deleted the sccs-info branch March 3, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better debug logs for borrowck constraint graph #104239

Better debug logs for borrowck constraint graph #104239

b-naber commented Nov 10, 2022 •

edited

Loading

rustbot commented Nov 10, 2022

This comment has been minimized.

Noratrieb commented Nov 10, 2022

b-naber commented Nov 10, 2022

bors commented Nov 11, 2022

jackh726 commented Dec 1, 2022

b-naber commented Dec 14, 2022

bors commented Dec 23, 2022

jackh726 commented Dec 26, 2022

This comment was marked as off-topic.

jackh726 commented Jan 18, 2023

This comment has been minimized.

jackh726 commented Jan 18, 2023

jackh726 commented Jan 18, 2023

jackh726 commented Feb 20, 2023

b-naber commented Feb 20, 2023

This comment has been minimized.

bors commented Feb 20, 2023

bors commented Feb 21, 2023

This comment has been minimized.

rust-timer commented Feb 21, 2023

jackh726 commented Feb 21, 2023

b-naber commented Feb 21, 2023

jackh726 left a comment

jackh726 Feb 21, 2023

jackh726 Feb 21, 2023

jackh726 Feb 21, 2023

b-naber Feb 21, 2023

b-naber commented Feb 21, 2023

bors commented Feb 21, 2023

Better debug logs for borrowck constraint graph #104239

Better debug logs for borrowck constraint graph #104239

Conversation

b-naber commented Nov 10, 2022 • edited Loading

rustbot commented Nov 10, 2022

This comment has been minimized.

Noratrieb commented Nov 10, 2022

b-naber commented Nov 10, 2022

bors commented Nov 11, 2022

jackh726 commented Dec 1, 2022

b-naber commented Dec 14, 2022

bors commented Dec 23, 2022

jackh726 commented Dec 26, 2022

This comment was marked as off-topic.

jackh726 commented Jan 18, 2023

This comment has been minimized.

jackh726 commented Jan 18, 2023

jackh726 commented Jan 18, 2023

jackh726 commented Feb 20, 2023

b-naber commented Feb 20, 2023

This comment has been minimized.

bors commented Feb 20, 2023

bors commented Feb 21, 2023

This comment has been minimized.

rust-timer commented Feb 21, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

jackh726 commented Feb 21, 2023

b-naber commented Feb 21, 2023

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Feb 21, 2023

Choose a reason for hiding this comment

jackh726 Feb 21, 2023

Choose a reason for hiding this comment

jackh726 Feb 21, 2023

Choose a reason for hiding this comment

b-naber Feb 21, 2023

Choose a reason for hiding this comment

b-naber commented Feb 21, 2023

bors commented Feb 21, 2023

b-naber commented Nov 10, 2022 •

edited

Loading