Skip to content

optimize NLL constraint propagation a little #49472

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

Merged
merged 5 commits into from
Mar 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions src/librustc_mir/borrow_check/nll/region_infer/dfs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,26 @@ use borrow_check::nll::region_infer::values::{RegionElementIndex, RegionValueEle
use syntax::codemap::Span;
use rustc::mir::{Location, Mir};
use rustc::ty::RegionVid;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::bitvec::BitVector;
use rustc_data_structures::indexed_vec::Idx;

pub(super) struct DfsStorage {
stack: Vec<Location>,
visited: BitVector,
}

impl<'tcx> RegionInferenceContext<'tcx> {
/// Creates dfs storage for use by dfs; this should be shared
/// across as many calls to dfs as possible to amortize allocation
/// costs.
pub(super) fn new_dfs_storage(&self) -> DfsStorage {
let num_elements = self.elements.num_elements();
DfsStorage {
stack: vec![],
visited: BitVector::new(num_elements),
}
}

/// Function used to satisfy or test a `R1: R2 @ P`
/// constraint. The core idea is that it performs a DFS starting
/// from `P`. The precise actions *during* that DFS depend on the
Expand All @@ -34,25 +51,29 @@ impl<'tcx> RegionInferenceContext<'tcx> {
/// - `Ok(false)` if the walk was completed with no changes;
/// - `Err(early)` if the walk was existed early by `op`. `earlyelem` is the
/// value that `op` returned.
pub(super) fn dfs<C>(&self, mir: &Mir<'tcx>, mut op: C) -> Result<bool, C::Early>
#[inline(never)] // ensure dfs is identifiable in profiles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep this in? Seems potentially bad for performance, though it does seem somewhat unlikely.

Copy link
Member

@pnkfelix pnkfelix Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(removed comment from review that was intended as response to @Mark-Simulacrum above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this would ever be criticial for performance, this is why I kept it in, but for sure it is not relevant now, given the data we're looking at. =) And it's convenient for profiling etc.

pub(super) fn dfs<C>(
&self,
mir: &Mir<'tcx>,
dfs: &mut DfsStorage,
mut op: C,
) -> Result<bool, C::Early>
where
C: DfsOp,
{
let mut changed = false;

let mut stack = vec![];
let mut visited = FxHashSet();

stack.push(op.start_point());
while let Some(p) = stack.pop() {
dfs.visited.clear();
dfs.stack.push(op.start_point());
while let Some(p) = dfs.stack.pop() {
let point_index = self.elements.index(p);

if !op.source_region_contains(point_index) {
debug!(" not in from-region");
continue;
}

if !visited.insert(p) {
if !dfs.visited.insert(point_index.index()) {
debug!(" already visited");
continue;
}
Expand All @@ -62,25 +83,27 @@ impl<'tcx> RegionInferenceContext<'tcx> {

let block_data = &mir[p.block];

let start_stack_len = stack.len();
let start_stack_len = dfs.stack.len();

if p.statement_index < block_data.statements.len() {
stack.push(Location {
dfs.stack.push(Location {
statement_index: p.statement_index + 1,
..p
});
} else {
stack.extend(block_data.terminator().successors().iter().map(
|&basic_block| {
Location {
dfs.stack.extend(
block_data
.terminator()
.successors()
.iter()
.map(|&basic_block| Location {
statement_index: 0,
block: basic_block,
}
},
));
}),
);
}

if stack.len() == start_stack_len {
if dfs.stack.len() == start_stack_len {
// If we reach the END point in the graph, then copy
// over any skolemized end points in the `from_region`
// and make sure they are included in the `to_region`.
Expand Down Expand Up @@ -229,9 +252,8 @@ impl<'v, 'tcx> DfsOp for TestTargetOutlivesSource<'v, 'tcx> {
// `X: ur_in_source`, OK.
if self.inferred_values
.universal_regions_outlived_by(self.target_region)
.any(|ur_in_target| {
self.universal_regions.outlives(ur_in_target, ur_in_source)
}) {
.any(|ur_in_target| self.universal_regions.outlives(ur_in_target, ur_in_source))
{
continue;
}

Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/borrow_check/nll/region_infer/dump_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
sub,
point,
span,
next: _,
} = constraint;
with_msg(&format!(
"{:?}: {:?} @ {:?} due to {:?}",
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/nll/region_infer/graphviz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<'this, 'tcx> dot::GraphWalk<'this> for RegionInferenceContext<'tcx> {
vids.into_cow()
}
fn edges(&'this self) -> dot::Edges<'this, Constraint> {
(&self.constraints[..]).into_cow()
(&self.constraints.raw[..]).into_cow()
}

// Render `a: b` as `a <- b`, indicating the flow
Expand Down
Loading