Improve query cycle errors for parallel queries #56434
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
ljedrz
reviewed
Dec 2, 2018
| @@ -326,19 +326,17 @@ fn connected_to_root<'tcx>( | |||
| query: Lrc<QueryJob<'tcx>>, | |||
| visited: &mut FxHashSet<*const QueryJob<'tcx>> | |||
| ) -> bool { | |||
| // This query is connected to the root (it has no query parent), return true | |||
| if query.parent.is_none() { | |||
There was a problem hiding this comment.
Isn't this early return check cheaper than the contains below?
There was a problem hiding this comment.
We want to return false here if query has already been visited instead of true.
|
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 |
|
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 |
src/librustc/ty/query/job.rs
Outdated
| query.info.query.hash_stable(&mut hcx, &mut stable_hasher); | ||
| let no_span = span == DUMMY_SP; | ||
| // Prefer entry points which have valid spans for nicer error messages | ||
| (no_span, stable_hasher.finish()) |
There was a problem hiding this comment.
This looks like things with no span would be preferred to things with span? Can you make no_span an integer so that one can easier see what's going on?
There was a problem hiding this comment.
Is let no_span = (span == DUMMY_SP) as u8 any better?
There was a problem hiding this comment.
I'd prefer something like:
// Sort span-less queries to the back
let span_cmp = if span == DUMMY_SPAN { 1 } else { 0 };| @@ -21,6 +21,7 @@ RUN sh /scripts/sccache.sh | |||
|
|
|||
| # using llvm-link-shared due to libffi issues -- see #34486 | |||
| ENV RUST_CONFIGURE_ARGS \ | |||
| --enable-experimental-parallel-queries \ | |||
There was a problem hiding this comment.
This shouldn't be committed, I guess.
There was a problem hiding this comment.
I wouldn't mind it too much =P
Zoxc
force-pushed
the
par-tests
branch
2 times, most recently
from
64673f1 to
57db5ad
Compare
|
@bors r=michaelwoerister |
|
📌 Commit 813b484 has been approved by |
bors
added
S-waiting-on-bors
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Dec 7, 2018
Improve query cycle errors for parallel queries r? @michaelwoerister
bors
added a commit
that referenced
this pull request
Dec 7, 2018
Rollup of 7 pull requests Successful merges: - #56000 (Add Armv8-M Mainline targets) - #56250 (Introduce ptr::hash for references) - #56434 (Improve query cycle errors for parallel queries) - #56516 (Replace usages of `..i + 1` ranges with `..=i`.) - #56555 (Send textual profile data to stderr, not stdout) - #56561 (Fix bug in from_key_hashed_nocheck) - #56574 (Fix a stutter in the docs for slice::exact_chunks) Failed merges: r? @ghost
eddyb
reviewed
Dec 7, 2018
| @@ -1942,8 +1942,12 @@ pub mod tls { | |||
| /// This is a callback from libsyntax as it cannot access the implicit state | |||
| /// in librustc otherwise | |||
| fn span_debug(span: syntax_pos::Span, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
There was a problem hiding this comment.
I am confused, isn't this only used at the same time as the TLS TyCtxt is present?
How can with_opt give you None? Did that code change somehow?
There was a problem hiding this comment.
With parallel queries we set this on the worker threads when we create the thread pool. In that case it acts it's more like runtime linking and it will be set when no TyCtxt is around.
There was a problem hiding this comment.
Why not set it at the same time as the TyCtxt TLS?
There was a problem hiding this comment.
We need to set this on all worker threads, which we can easily do when creating the thread pool.
We could put it in ImplicitCtxt, but it would never change, so that would just make queries slower for no reason.
There was a problem hiding this comment.
In that case, can we make it so the debug callbacks are all set globally before entering even the global TyCtxt even without parallel queries?
There was a problem hiding this comment.
Maybe we just need a scoped_thread_local for the CodeMap? I feel like that would probably be a cleaner design.
|
💥 Test timed out |
bors
added
S-waiting-on-review
kennytm
removed
the
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @michaelwoerister