A fix for 51821 #51855
A fix for 51821 #51855
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| type_tests, | ||
| universal_regions, | ||
| }; | ||
|
|
||
| for c in outlives_constraints { | ||
| result.add_outlives_iner(c); |
| } | ||
| } | ||
|
|
||
| pub fn iner(&self) -> &IndexVec<ConstraintIndex, OutlivesConstraint> { |
There was a problem hiding this comment.
Nit: "inner" -- but this method seems surprising, perhaps it'll disappear in future commits though =)
There was a problem hiding this comment.
I will fix the typo.
As to removing it in the future, iif that is what you want to get this merged. The alternative is to wrap all the iter methods that get used all over the place.
|
Just dropping a link to #51821 for easy navigation :) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I expect impl ConstraintSet {
fn new() -> Self { .. }
fn push(&mut self, constraint: Constraint) { .. }
fn link(&mut self) { /* updates the `next` fields */ }
fn each_affected_by_dirty(&mut self, r: RegionVid, op: impl FnMut(ConstraintIndex)) {
/* given that the region `a` has changed, invoke `op` with each constraint `b: a` */
}
} |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| } | ||
| } | ||
|
|
||
| pub fn inner(&self) -> &IndexVec<ConstraintIndex, OutlivesConstraint> { |
There was a problem hiding this comment.
Nit: Instead of Inner, we could implement IndexVec for ConstraintSet. Something like this;
impl Index<ConstraintIndex> for ConstraintSet {
type Output = OutlivesConstriant;
fn index(&self, index: ConstraintIndex) -> &OutlivesConstraint {
&self.constraints[index]
}
}then you can just do constraint_set[index] instead of constraint_set.inner()[index]
| &self.constraints | ||
| } | ||
|
|
||
| pub fn link(&mut self, len: usize) -> IndexVec<RegionVid, Option<ConstraintIndex>> { |
There was a problem hiding this comment.
Let's add a comment here:
Once all constraints have been added, link() is used to thread together the constraints based on which would be affected when a particular region changes. See the next field of OutlivesContraint for more deatils. link returns a map that is needed later by each_affected_by_dirty.
| map | ||
| } | ||
|
|
||
| pub fn each_affected_by_dirty( |
There was a problem hiding this comment.
Comment:
When a region R1 changes, we need to reprocess all constraints R2: R1 to take into account any new elements that R1 now has. This method will quickly enumerate all such constraints (that is, constraints where R1 is in the "subregion" position). To use it, invoke with map[R1] where map is the map returned by link; the callback op will be invoked for each affected constraint.
| @@ -494,20 +462,18 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
| while let Some(constraint_idx) = dirty_list.pop() { | |||
| clean_bit_vec.insert(constraint_idx.index()); | |||
|
|
|||
| let constraint = &self.constraints[constraint_idx]; | |||
| let constraint = &self.constraints.inner()[constraint_idx]; | |||
There was a problem hiding this comment.
this would be outlives_constraints[constraint_idx] with the Index impl I proposed
| @@ -971,7 +928,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
| ); | |||
|
|
|||
| let blame_index = self.blame_constraint(longer_fr, shorter_fr); | |||
| let blame_span = self.constraints[blame_index].span; | |||
| let blame_span = self.constraints.inner()[blame_index].span; | |||
| @@ -1062,7 +1019,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
| // - `fr1: X` transitively | |||
| // - and `Y` is live at `elem` | |||
| let index = self.blame_constraint(fr1, elem); | |||
| let region_sub = self.constraints[index].sub; | |||
| let region_sub = self.constraints.inner()[index].sub; | |||
|
Actually, maybe we should just remove impl Deref for ConstraintSet {
type Target = IndexVec<ConstraintIndex, OutlivesConstraint>;
fn deref(&self) -> &Self::Target { &self.constriants }
}then you can do things like It seems pretty reasonable to say that a constraint set can be viewed as an |
|
r=me once travis is happy |
|
do you want to profile/benchmark to confirm that this had the intended effect? |
|
Travis is happy. |
|
☔ The latest upstream changes (presumably #51538) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Eh2406 merge conflicts :( Regarding profiling, we could -- but we also profile the effect of every PR that lands, so either way we'll find out, and it's hard to imagine how this could make things worse. |
|
I did a rebase and pushed. I am getting a local error, but it is something really random so, I think it is just me. |
|
@bors r+ |
|
📌 Commit 6e0cefe has been approved by |
bors
added
S-waiting-on-bors
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
the
S-waiting-on-author
|
@bors retry |
|
⌛ Trying commit 6e0cefe with merge 9bc7781a7a9a89400bceb10d87ac3580e61c78f8... |
|
☀️ Test successful - status-travis |
|
cc @Mark-Simulacrum ok, bors try is done, perf run please 😁 |
|
Perf queued. |
|
do we have a ETA on perf? |
|
About 2 hours, maybe a little less. |
kennytm
removed
the
S-waiting-on-perf
|
Kind of surprising how little difference this makes =( Still, I think the code is nicer this way. I'm still inclined to r+ |
|
Looking at some graphs, I think I see why this has minimal impact. Most of the duplicate edges I see appear to be self loops ( pub fn merge(&mut self, read: R, write: R) -> bool {
let mut changed = false;
if read != write {
let (bit_set_read, bit_set_write) = self.vector.pick2_mut(read, write);
for read_chunk in bit_set_read.chunks() {
changed = changed | bit_set_write.insert_chunk(read_chunk).any();
}
}
changed
} |
|
So my estimate was mistaken. |
It turns out that we don't have duplicates, just self-cycles.
|
@bors r+ OK, so, although this didn't yield the perf win I expected, I think it's nice to have the constraint set extracted, and I also think it might lay the groundwork for some future changes I was contemplating experimenting with. However, since the hashset is not really helping us, I took the liberty of removing it with a quick commit. I have some ideas for follow-up work here though it will be a bit more involved. I may take a stab at it today if I have some free time -- we'll see though -- but otherwise I'll open an issue. @Eh2406, maybe you'd be interested in doing that? |
|
📌 Commit ac5bd5d has been approved by |
bors
added
S-waiting-on-bors
A fix for 51821 This dedupe the vec of `OutlivesConstraint` using a `FxHashSet<(RegionVid, RegionVid)>` it alsow adds a `struct ConstraintSet` to encapsulate/ensure this behavere.
|
☀️ Test successful - status-appveyor, status-travis |
|
Feel free to ping me, but don't wait on me. :-) |
This dedupe the vec of
OutlivesConstraintusing aFxHashSet<(RegionVid, RegionVid)>it alsow adds astruct ConstraintSetto encapsulate/ensure this behavere.