Instrument accesses to the HIR Map #31304
Conversation
|
☔ The latest upstream changes (presumably #31250) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So I did some measurements to judge the effect of the various changes:
These are based on measuring times for libsyntax.
So, basically, none. |
|
Note: I don't have any tests here because I'm not aware of a particular test that would fail at the moment thanks to the use of the HIR map. |
|
But it still seems clear it should be instrumented. :) |
nikomatsakis
force-pushed
the
incr-comp-read-from-hir-map
branch
2 times, most recently
from
67a7a57 to
7a87f98
Compare
| // This function reveals the name of the item and hence is a | ||
| // kind of read. This is inefficient, since it walks ancestors | ||
| // and we are walking them anyhow, but whatever. | ||
| self.read(id); |
There was a problem hiding this comment.
So, I've changed my mind about this one: The read should be there. If something depends on the name, it must be invalidated by a name change. The only thing different when changing the name (as opposed to changing some other part of the item) is that the change will look to the system like them item was removed instead of altered. But that does not change the fact that we need the edge to properly invalidate things depending on it.
There was a problem hiding this comment.
@michaelwoerister I seem to recall going through a very similar line of thinking when I wrote that call in the first place; maybe I will expand the comment a bit.
|
Looking good |
so that we can still get assertion failures even when dep-graph construction is disabled.
another and were not previously instrumented.
nikomatsakis
force-pushed
the
incr-comp-read-from-hir-map
branch
from
7a87f98 to
a0f96d6
Compare
|
@bors r=michaelwoerister |
|
📌 Commit a0f96d6 has been approved by |
…chaelwoerister This change also modifies the dep graph infrastructure to keep track of the number of active tasks, so that even if we are not building the full dep-graph, we still get assertions when there is no active task and one does something that would add a read/write edge. This is particularly helpful since, if the assertions are *not* active, you wind up with the error happening in the message processing thread, which is too late to know the correct backtrace. ~~Before landing, I need to do some performance measurements. Those are underway.~~ See measurements below. No real effect on time. r? @michaelwoerister
This change also modifies the dep graph infrastructure to keep track of the number of active tasks, so that even if we are not building the full dep-graph, we still get assertions when there is no active task and one does something that would add a read/write edge. This is particularly helpful since, if the assertions are not active, you wind up with the error happening in the message processing thread, which is too late to know the correct backtrace.
Before landing, I need to do some performance measurements. Those are underway.See measurements below. No real effect on time.
r? @michaelwoerister