Better debug logs for borrowck constraint graph by b-naber · Pull Request #104239 · rust-lang/rust

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

fnwith_reg_var_to_origin(&self,f:implFnOnce(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
fnwith_reg_var_to_origin(&self,f:implFnOnce(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

- 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

 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

 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 acea19cCompare November 10, 2022 13:23

b-naber force-pushed the sccs-info branch from acea19c to a179d1bCompare 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 e794e12Compare December 14, 2022 20:38

b-naber force-pushed the sccs-info branch from e794e12 to 269f1e0Compare 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 570ad62Compare 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-vmsBotforce-pushed the sccs-info branch from 0e917b4 to c9843d6Compare 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:masterFeb 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

Uh oh!

Uh oh!

Conversation

b-naber commented Nov 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 10, 2022

Uh oh!

This comment has been minimized.

Noratrieb commented Nov 10, 2022

Uh oh!

b-naber commented Nov 10, 2022

Uh oh!

bors commented Nov 11, 2022

Uh oh!

jackh726 commented Dec 1, 2022

Uh oh!

b-naber commented Dec 14, 2022

Uh oh!

bors commented Dec 23, 2022

Uh oh!

jackh726 commented Dec 26, 2022

Uh oh!

This comment was marked as off-topic.

jackh726 commented Jan 18, 2023

Uh oh!

This comment has been minimized.

jackh726 commented Jan 18, 2023

Uh oh!

jackh726 commented Jan 18, 2023

Uh oh!

jackh726 commented Feb 20, 2023

Uh oh!

b-naber commented Feb 20, 2023

Uh oh!

This comment has been minimized.

bors commented Feb 20, 2023

Uh oh!

bors commented Feb 21, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Feb 21, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

jackh726 commented Feb 21, 2023

Uh oh!

b-naber commented Feb 21, 2023

Uh oh!

jackh726 left a comment

Choose a reason for hiding this comment

Uh oh!

jackh726Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

jackh726Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

jackh726Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

b-naberFeb 21, 2023

Choose a reason for hiding this comment

Uh oh!

b-naber commented Feb 21, 2023

Uh oh!

bors commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

b-naber commented Nov 10, 2022•
edited
Loading