Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
That's not how that works. |
There was a problem hiding this comment.
I've been looking at enough of this code to have some idea of what's going on now. This new code is definitely easier to follow. E.g. the old comments at the top of visit_waiters were really hard to understand. The remaining code is still very complicated!
There are enough changes that I'm having trouble seeing the exact 1-to-1 mapping from the old control flow to the new control flow. Especially given that it seems (from the PR description) there are some deliberate changes to the control flow.
This file uses "query" in a lot of places where "job" would probably be better. (Working on the assumption that "query" the conceptual thing, like type_of, and "job" is an actively running query with a particular key.) That's beyond the scope of this PR, though.
| struct Waiter { | ||
| span: Span, | ||
| /// The query which we are waiting from, if none the waiter is from a compiler root. | ||
| query: Option<QueryJobId>, |
There was a problem hiding this comment.
Should this be called parent? (I have been wondering the same thing about QueryWaiter::query.
| /// The query which we are waiting from, if none the waiter is from a compiler root. | ||
| query: Option<QueryJobId>, | ||
| resumable: Option<ResumableWaiterLocation>, | ||
| } |
There was a problem hiding this comment.
We now have QueryWaiter and Waiter. And also QueryJobInfo and QueryInfo. These structs with similar names and contents are hard to remember.
There was a problem hiding this comment.
Also, the variable name waiter now sometimes means a QueryWaiter, sometimes a Waiter, and sometimes a QueryJobId. This gets quite confusing. It would be good to have separate names. E.g. sometimes waiter_query is used for a QueryJobId that comes from waiter.query, which is better.
| result.push(Waiter { | ||
| span: waiter.span, | ||
| query: waiter.query, | ||
| resumable: Some((query, i)), |
There was a problem hiding this comment.
You're pushing the same query multiple times. I think it's possible to change ResumableWaiterLocation to just be the usize index, because the query is always available at the waiters_of call site.
There was a problem hiding this comment.
Actually, I'm not sure if that works. It's still odd to see query pushed multiple times.
| /// the function return true. | ||
| /// If a cycle was not found, the starting query is removed from `jobs` and | ||
| /// the function returns false. | ||
| fn remove_cycle<'tcx>( |
There was a problem hiding this comment.
This function is a good example of confusing names: waiter appears multiple times, with a different type and meaning each time.
This replaces
visit_waiterswhich uses closures to visit waiters withwaiters_ofwhich returns a list of waiters. This makes the control flow of their users a bit clearer.Additionally
waiters_ofincludes non-query waiters unlikevisit_waiters. This means thatconnected_to_rootnow considers resumable waiters from the compiler root as being connected to root, while previously only non-resumable waiters counted. This can result in some additional entry points being found. Similarly in the loop detecting entry points we now consider queries in the cycle with direct resumable waiters from the compiler root as being entry points.When looking for entry points we now look at waiters until we found a query to populate the
EntryPoint.waiterfield instead of stopping when we determined it to be an entry point.cc @petrochenkov @nnethercote