Skip to content

[nll] make causal tracking lazy #51710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nikomatsakis opened this issue Jun 22, 2018 · 4 comments
Closed

[nll] make causal tracking lazy #51710

nikomatsakis opened this issue Jun 22, 2018 · 4 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-performant Working towards the "performance is good" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Analyzing profiles on my latest branch, it appears that approximately 4% of total compilation time is spent managing the "causal hash map":

/// If cause tracking is enabled, maps from a pair (r, e)
/// consisting of a region `r` that contains some element `e` to
/// the reason that the element is contained. There should be an
/// entry for every bit set to 1 in `SparseBitMatrix`.
causes: Option<CauseMap>,

Currently, that map is only non-None for the "liveness" set, which stores, for each region variable R, each point P where R is directly live (meaning typically that there is some local variable X that has R in its type and which may be used at some point Q reachable from P). The "cause" tracks basically this point Q. It's accessed later when constructing error messages, here:

// then return why `Y` was live at `elem`
self.liveness_constraints.cause(region_sub, elem)

We should remove the causal hashmap altogether. Instead, when constructing an error, we can do a BFS over the MIR graph from elem (a Location), looking for points that may use the region region_sub that we are interested in.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll NLL-performant Working towards the "performance is good" goal labels Jun 22, 2018
@nikomatsakis
Copy link
Contributor Author

cc @rust-lang/wg-compiler-nll -- possible 4% win

@nikomatsakis
Copy link
Contributor Author

Here is a thread for chatting about this on zulip:

https://rust-lang.zulipchat.com/#narrow/stream/122657-wg-nll/topic/issue-51710-lazy-causal-tracking

@spastorino spastorino self-assigned this Jun 22, 2018
@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-NLL Area: Non-lexical lifetimes (NLL) labels Jun 26, 2018
@nikomatsakis
Copy link
Contributor Author

So on Zulip @spastorino pointed out to me that we already have the BFS I described:

fn find_regular_use<'gcx, 'tcx>(
mir: &'gcx Mir,
regioncx: &'tcx RegionInferenceContext,
borrow: &'tcx BorrowData,
start_point: Location,
local: Local,
) -> Option<Location> {
let mut uf = UseFinder {
mir,
regioncx,
borrow,
start_point,
local,
liveness_mode: LivenessMode {
include_regular_use: true,
include_drops: false,
},
};
uf.find()
}
fn find_drop_use<'gcx, 'tcx>(
mir: &'gcx Mir,
regioncx: &'tcx RegionInferenceContext,
borrow: &'tcx BorrowData,
start_point: Location,
local: Local,
) -> Option<Location> {
let mut uf = UseFinder {
mir,
regioncx,
borrow,
start_point,
local,
liveness_mode: LivenessMode {
include_regular_use: false,
include_drops: true,
},
};
uf.find()
}

I think this perhaps suggests a simpler way to achieve the same goal. What I originally described was that we would use the BFS to basically "reproduce" the same return value we have now -- but that return value isn't ideal.

Let's step back to explain how things work today. We have this function explain_why_region_contains_point(R, P). It has the job of figuring out why the point P is part of the region R. What we are returning now is a cause like LiveLocal(X, P1), which says that the variable X may contain a reference to data in region R -- or possibly in some other region R1 where R: R1 -- and that it is live at P1. But being live isn't the same as being used -- being live just means it will be used in the future. So then we do a BFS to find the point where it is used, since that is what we want to show to the user. If we naively integrated the BFS into the current setup, we would wind up with two BFS's. That is a bit silly.

I can see two ways forward:

  • Move the BFS from where it is now into explain_why_region_contains_point. This is basically what I described. We would be searching then not for a specific local variable X but rather for uses of any local variable that contains region_sub (R1 in my description above). We would then return both the variable name (X) and the point where it is used (P). This simplifies the code later since it isn't finding out where X is live but rather where it is used.

  • Leave the BFS where it is, but modify the return value so that it retuns region_sub. This is basically the same as the previous paragraph, just differs in the precise info returned by each function.

I guess I lean towards the former. Feels 'more right' -- explain_why_region_contains_point is just doing a better job of finding the answer.

bors added a commit that referenced this issue Jun 29, 2018
@nikomatsakis nikomatsakis added this to the Rust 2018 Preview 2 milestone Jun 29, 2018
@nikomatsakis
Copy link
Contributor Author

This has a pending PR, marking as Preview 2.

bors added a commit that referenced this issue Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-performant Working towards the "performance is good" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants