Add tombstone checks to make sure incr-comp reads aren't dirtied #40362
Conversation
| @@ -134,7 +175,7 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> { | |||
| /// accesses the body of the item `item`, so we register a read | |||
| /// from `Hir(item_def_id)`. | |||
| fn memoize<OP>(&self, key: M::Key, op: OP) -> M::Value | |||
| where OP: FnOnce() -> M::Value | |||
| where OP: FnOnce() -> Option<M::Value> | |||
There was a problem hiding this comment.
it looks to me like there is no longer a need for this part of the diff -- that is, we can make memoize() take a closure that returns just M::Value again
There was a problem hiding this comment.
It's a little hard to see because of how the diff reads on Github, but this call to memoize has a closure that returns None.
There was a problem hiding this comment.
Ah, I thought @eddyb refactored that use. I wonder if we want to do so in this PR instead. Right now what happens is that we request AssociatedItem(X), which is some summary information about some item X that is part of an impl or trait:
impl Foo {
type A;
type X;
fn b() { ... }
}To do this, we go up to the enclosing impl (the one shown there) and then iterate over its list of items. Right now, since we are iterating anyway, we would compute the associated item info for A and b() while we're at it (to avoid an O(n^2) blow-up, basically). We have been talking though about trying to refactor into finer-grained tasks that don't have this idea of generating "N results" from one execution.
We could thus change this code to not generate N outputs but just find the one item we actually want. It would be O(N^2) but N here is typically very small.
And we do plan to rework things over time that would mitigate -- for example, moving to a newer system where we wouldn't have to be so careful about our dependencies. The whole motivation here was to only read from the impl when computing this data, and not the items themselves. This way, if the item X changes, we don't have to consider the value as dirty (the parts of X that would affect this value are copied into the impl and hashed there too). But we want to move to a system where we would recompute this value (which is cheap) and then see that, indeed, it has not changed.
There was a problem hiding this comment.
The best fix IMO is to move AssociatedItem into the HIR where it belongs.
| // this is the memoization pattern: for that, use `memoize()` | ||
| // below | ||
| #[cfg(debug_assertions)] | ||
| assert!(!self.tombstones.borrow().contains(&k), |
There was a problem hiding this comment.
The following lines should probably be wrapped in a block.
There was a problem hiding this comment.
Oh, I misread. But a debug_assert() would be more concise.
|
All right, let's land this. I have been doing some investigation and these changes would definitely be useful. We can always factor away the |
|
@bors r+ |
|
📌 Commit 1ef95aa has been approved by |
|
@bors r- |
|
Actually, what I said is still true, but I'd like a chance to do a bit more experiments, maybe chat with @cramertj a bit -- after all, this diff is fairly minor, will be easily rebased. |
|
☔ The latest upstream changes (presumably #39628) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this and instead we'll pursue the strategy of removing the use of methods other than |
Part of #40305.
r? @nikomatsakis