Remove nodes_in_current_session field and related assertions#154283
Remove nodes_in_current_session field and related assertions#154283Zoxc wants to merge 2 commits intorust-lang:mainfrom
nodes_in_current_session field and related assertions#154283Conversation
rustbot
added
S-waiting-on-review
|
rustbot has assigned @jdonszelmann. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
Zoxc
force-pushed
the
rem-nodes_in_current_session
branch
from
922660c to
267d6cf
Compare
|
This looks reasonable to me, but I'm no expert on the query system. Going to defer to @nnethercote (or someone else if that's better) r? @nnethercote |
| } | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| fn record_edge( |
There was a problem hiding this comment.
I'd prefer to keep this entire function cfg(debug_assertions), and then likewise at the call sites. It makes it clearer that it's all debug-only, and avoids the need for the underscores on the parameter names.
There was a problem hiding this comment.
I was thinking of just converting this to cfg!.
There was a problem hiding this comment.
#[cfg] makes the call sites clearer.
There was a problem hiding this comment.
Why?
I basically always prefer if cfg! to #[cfg] when technically possible because it doesn't hide the configured-out code from type checker and other later passes.
There was a problem hiding this comment.
Fine, whatever, just do cfg! then.
There was a problem hiding this comment.
Should I do it in this PR? I was thinking of making a follow-up PR.
There was a problem hiding this comment.
Just do it here, save the trouble of filing a follow-up
nnethercote
added
S-waiting-on-author
Zoxc
force-pushed
the
rem-nodes_in_current_session
branch
from
267d6cf to
14b2173
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| forbidden_edge, | ||
| #[cfg(debug_assertions)] | ||
| value_fingerprints: Lock::new(IndexVec::from_elem_n(None, new_node_count_estimate)), | ||
| value_fingerprints: Lock::new(if cfg!(debug_assertions) { |
There was a problem hiding this comment.
Now we are always doing work that we were only doing in debug builds, because we have fields in the struct that we used to only have in debug builds.
Sorry, I didn't realize that switching to cfg! would cause this. I think this goes too far. I suggest just removing this second commit and going with the original first commit which uses #[cfg(debug_attributes)] within record_edge.
There was a problem hiding this comment.
I am tempted to add a DebugOnly type so we can have fields only present with debug_assertions, but still get type checking. That would be useful elsewhere too. Would that be sufficient given that the extra work would optimize away?
There was a problem hiding this comment.
Not sure I like that, but it would definitely be a separate PR anyway.
6 participants
This removes the
nodes_in_current_sessionfield and related assertions. These are enabled if-Z incremental-verify-ichis passed ordebug_assertionsis on. Historically these have been useful to catch query keys with improperHashStableimpls which lead to collisions.We currently also check for duplicate nodes when loading the dep graph. This check is more complete as it covers the entire dep graph and is enabled by default. It doesn't provide a query key for a collision however. This check is also delayed to the next incremental session.
We also have the
verify_query_key_hasheswhich is also enabled if-Z incremental-verify-ichis passed ordebug_assertionsis on. This checks for dep node conflicts in each query cache and provides 2 conflicting keys if present.I think these remaining checks are sufficient and so we can remove
nodes_in_current_session.