Make region inference use a dirty list #47766
Conversation
| }, | ||
| ); | ||
| let dependency_map = self.build_dependency_map(); | ||
| let mut dirty_list = self.constraints.clone(); |
There was a problem hiding this comment.
Nit: I think the dirty list ought to be either refs to constraints or indices of constraints. Since you used indices for the dependency-map, let's go with those. (It'll also avoid any borrowing issues.)
So something like:
let mut dirty_list: Vec<_> = (0..self.containts.len()).collect();| let mut dirty_list = self.constraints.clone(); | ||
|
|
||
| debug!("propagate_constraints: --------------------"); | ||
| while let Some(constraint) = dirty_list.pop() { |
There was a problem hiding this comment.
while let Some(constraint_index) = dirty_list.pop() {
let constraint = &self.constraints[constraint_index];
...
}| debug!("propagate_constraints: sup={:?}", constraint.sup); | ||
|
|
||
| if let Some(constraint_deps) = dependency_map.get(&constraint.sup) { | ||
| for dep_idx in constraint_deps { |
There was a problem hiding this comment.
Now this can be
if let Some(constraint_deps) = dependency_map.get(&constraint.sup) {
dirty_list.extend(constraint_deps);
}Not that it matters much, but generally invoking extend is better than for loop that calls push. This is because sometimes the iterator knows how long it will be, which lets dirty_list reallocate its backing array more efficiently.
Finally, you could simplify this further, like so:
dirty_list.extend(
dependency_map.get(&constraint.sup).unwrap_or(&vec![])
);but I'm not sure that's really clearer (arguably less so). The key here is that creating an empty vector (i.e., vec![]) is basically free -- it's just some null values, no allocation or anything. In the case where I have a "map to vec", I often do this style, under the idea that a missing entry is equivalent to an empty vector. But either way really.
spastorino
force-pushed
the
inference-dirty-list
branch
from
b1ed025
to
50280ea
Compare
|
@nikomatsakis applied all the requested changes |
kennytm
added
the
S-waiting-on-review
|
Have you run Clippy on your changes? |
|
@leonardo-m why would we do that? is there some specific lint you are imagining? fwiw, we don't usually run clippy on the compiler. |
| } | ||
| debug!("propagate_constraints: --------------------"); | ||
| while let Some(constraint_idx) = dirty_list.pop() { | ||
| let constraint = self.constraints[constraint_idx]; |
There was a problem hiding this comment.
this could be &self.constraints[constraint_idx] -- no reason to copy the value out
| debug!("propagate_constraints: sup={:?}", constraint.sup); | ||
|
|
||
| dirty_list.extend( | ||
| dependency_map.get(&constraint.sup).unwrap_or(&vec![]) |
There was a problem hiding this comment.
as you said, it might be nice to have a bitvec storing what is already present in the dirty_list, which you would then have to clear each time we pop something out of the dirty list.
spastorino
force-pushed
the
inference-dirty-list
branch
from
50280ea
to
6b01707
Compare
|
@nikomatsakis ready! |
I didn't know that Clippy is not used on the compiler. Clippy gives lot of false positives, but some advice is good, so perhaps you should use Clippy...
I think Clippy often suggests to replace or_insert with or_insert_with |
Ah. This case is better left as |
| debug!("propagate_constraints: sup={:?}", constraint.sup); | ||
| changed = true; | ||
| for dep_idx in dependency_map.get(&constraint.sup).unwrap_or(&vec![]) { | ||
| if !dirty_bit_vec.contains(*dep_idx) { |
There was a problem hiding this comment.
you can change this to:
if dirty_bit_vec.insert(*dep_idx) {
dirty_list.push(*dep_idx);
}and save a bit of execution time.
| debug!("propagate_constraints: sub={:?}", constraint.sub); | ||
| debug!("propagate_constraints: sup={:?}", constraint.sup); | ||
| changed = true; | ||
| for dep_idx in dependency_map.get(&constraint.sup).unwrap_or(&vec![]) { |
There was a problem hiding this comment.
Nit: I'd usually write for &dep_idx in ... just so you don't need the *dep_idx below
| debug!("\n"); | ||
| } | ||
|
|
||
| self.inferred_values = Some(inferred_values); | ||
| } | ||
|
|
||
| fn build_dependency_map(&self) -> HashMap<RegionVid, Vec<usize>> { |
There was a problem hiding this comment.
Maybe a comment here would be nice, actually. You could steal the stuff I wrote in the issue description (i.e., the example). But something like this would suffice:
Builds up a map from each region variable X to a vector with the indices of constraints that need to be re-evaluated when X changes. These are constraints like Y: X @ P -- so if X changed, we may need to grow Y.
spastorino
force-pushed
the
inference-dirty-list
branch
from
6b01707
to
da545ce
Compare
I suspect the lines would be long for tidy.
| ); | ||
| let dependency_map = self.build_dependency_map(); | ||
| let mut dirty_list: Vec<_> = (0..self.constraints.len()).collect(); | ||
| let mut dirty_bit_vec = BitVector::new(dirty_list.len()); |
There was a problem hiding this comment.
Nit: this starts as all zeroes, but we really want it to start as all ones I suppose. But it won't be wrong this way, just mildly less efficient. Actually, we could "invert the sense" of the vector, and have it store a 1 if the thing is not present....call it clean_bit_vec...
|
|
||
| debug!("propagate_constraints: --------------------"); | ||
| while let Some(constraint_idx) = dirty_list.pop() { | ||
| dirty_bit_vec.remove(constraint_idx); |
There was a problem hiding this comment.
...in that case, this would be clean_bit_vec.insert(constraint_idx);...
| debug!("propagate_constraints: sup={:?}", constraint.sup); | ||
| changed = true; | ||
| for &dep_idx in dependency_map.get(&constraint.sup).unwrap_or(&vec![]) { | ||
| if dirty_bit_vec.insert(dep_idx) { |
There was a problem hiding this comment.
...and this would be if clean_bit_vec.remove(dep_idx) {
Otherwise the vector is initially out of sync
|
My measurements here locally using the testcase from #47267:
So this looks like a solid win for NLL to me! :) These are averages of 6 runs. The results were fairly stable but jumped around some. The test case is kind of small. So take them with a grain of salt. Here is a gist with my full measurements in case you are curious, each one is labeled with the SHA1 commit hash and whether it had NLL enabled or disabled. |
|
@bors r+ |
|
📌 Commit 205eba8 has been approved by |
The next Nightly I'll test this with a much larger amount of code. |
|
@leonardo-m if you have a test, I can run it now. |
|
⌛ Testing commit 205eba8 with merge 4349bc51453d0c74495589c8036622a9b1a05726... |
|
💔 Test failed - status-appveyor |
|
⌛ Testing commit 205eba8 with merge b7d4de9edf202b3a9cd889bf038ee663aecaf573... |
|
💔 Test failed - status-travis |
kennytm
added
S-waiting-on-bors
Make region inference use a dirty list r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
r? @nikomatsakis