Add placeholder types #55921
Merged
Add placeholder types #55921
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 has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
scalexm
force-pushed
the
placeholders
branch
from
77ba32d to
1736391
Compare
nikomatsakis
approved these changes
Nov 14, 2018
| ty::Infer(ty::TyVar(_)) => self.canonicalize_ty_var(CanonicalTyVarKind::General, t), | ||
| ty::Infer(ty::TyVar(vid)) => { | ||
| match self.infcx.unwrap().probe_ty_var(vid) { | ||
| // `t` could be a float / int variable: canonicalize that instead |
There was a problem hiding this comment.
I guess this is a bug fix?
There was a problem hiding this comment.
(Seems unrelated to the rest of the commit)
There was a problem hiding this comment.
Er, wait, won't canonical_ty_var handle this already?
There was a problem hiding this comment.
i.e., this logic here:
rust/src/librustc/infer/canonical/canonicalizer.rs
Lines 578 to 582 in 6f244c9
There was a problem hiding this comment.
oh, I guess the goal was to extract the ui?
There was a problem hiding this comment.
Yes the goal was to extract the ui!
|
@bors r+ |
|
📌 Commit 1736391 has been approved by |
bors
added
S-waiting-on-bors
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Nov 16, 2018
Add placeholder types Fixes rust-lang#48696 (handle universes in canonicalization of type inference vars), and fixes rust-lang#55098.
|
⌛ Testing commit 1736391 with merge 61e90d3fdf4b5a426b3a483c545e0be6f28cbbf4... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
bors
added
S-waiting-on-review
|
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 |
|
Same error, should be legit. |
kennytm
added
S-waiting-on-author
|
Anyone here who might have already dealt with that test? It seems that the faulty line is somehow cut, e.g. in the debug output there are things like: but the fact that the travis log is skipping everything before that faulty line is not helping. |
|
Also now I remember than in a previous PR, I had this test failing locally but not on travis. |
|
Ok so I've just understood that the rust/src/tools/compiletest/src/runtest.rs Line 3383 in 4632cf2 What happens is that some bytes are skipped in the output, and very unfortunately the output resumes at rust/src/tools/compiletest/src/json.rs Lines 79 to 92 in 4632cf2 So I guess this is actually the same as #53332. Btw that above code seems very hacky and I think this should be fixed instead of trying to work around |
|
cc @kennytm see my above hack, which according to this code: rust/src/tools/compiletest/src/runtest.rs Lines 2690 to 2709 in 4632cf2 should prevent |
|
Let's try. @bors r=nikomatsakis |
|
📌 Commit b8a30f0 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
4 tasks
|
@rust-lang/wg-compiler-performance: This caused a major performance regression. On the Here is a Cachegrind diff for a "clean debug" build of The heart of the increase is these lines: I would guess that The counts on the lines with the braces indicate that almost 9% of all instructions executed are occurring while entering and leaving this function. Therefore, marking I suspect it will be possible to improve this expression: by introducing a variant of In my opinion this PR should be backed out until the performance regression can be rectified. |
|
Instructions on how to run |
|
Thanks @nnethercote for this detailed report. |
bors
added a commit
that referenced
this pull request
Nov 26, 2018
Create multiple universes at once Related to [this](#55921 (comment)) comment cc @rust-lang/infra , could I have a try + perf run on that PR? Thanks!
|
@nnethercote do you know offhand if PR #56251 addressed the performance regression that you identified above? Update: if nothing else, this chart looks promising. (It compares the state prior to the identified regression to the state after PR #56251 landed, I think...) |
|
I compared 2018-11-25 (37961db) against 2018-11-25 (abe19a7), and there was a big regression for The "this chart" you mentioned compares 2018-11-25 (37961db) against 2018-11-27 (aeff91d), i.e. it's the same starting point, but a later end point. There is no regression, and a lot of improvements. So something has fixed the problem. A more obvious way to see it is by consulting perf.rust-lang.org: |
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.
Fixes #48696 (handle universes in canonicalization of type inference vars), and fixes #55098.