Skip to content

traits/fulfill: allow stalled_on to track ty::Const::Infer(_) (unused yet).#70213

Merged
bors merged 3 commits intorust-lang:masterfrom
eddyb:stalled-on-ty-or-const
Mar 24, 2020
Merged

traits/fulfill: allow stalled_on to track ty::Const::Infer(_) (unused yet).#70213
bors merged 3 commits intorust-lang:masterfrom
eddyb:stalled-on-ty-or-const

Conversation

@eddyb
Copy link
Member

@eddyb eddyb commented Mar 21, 2020

This PR addresses the representation side of #70180, but only actually collects ty::Infers via Ty::walk into stalled_on (collecting ty::ConstKind::Infers requires #70164).

However, it should be enough to handle #70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no walk-ing is involved).

This is my second attempt, see #70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2020
@eddyb
Copy link
Member Author

eddyb commented Mar 21, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Mar 21, 2020

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Mar 21, 2020

⌛ Trying commit 78c178b with merge cd27e54b181fec28e8c3e2b457e51762e307281b...

@eddyb eddyb mentioned this pull request Mar 21, 2020
Closed
@bors
Copy link
Collaborator

bors commented Mar 21, 2020

☀️ Try build successful - checks-azure
Build commit: cd27e54b181fec28e8c3e2b457e51762e307281b (cd27e54b181fec28e8c3e2b457e51762e307281b)

@rust-timer
Copy link
Collaborator

rust-timer commented Mar 21, 2020

Queued cd27e54b181fec28e8c3e2b457e51762e307281b with parent 5f13820, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Mar 21, 2020

Finished benchmarking try commit cd27e54b181fec28e8c3e2b457e51762e307281b, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Mar 21, 2020

@nnethercote ^^ Can you help me make sense of this? The query timings say a different story (the "regressions" actually spend less time in typeck_tables_of!) than the instructions:u.

@eddyb
Copy link
Member Author

eddyb commented Mar 21, 2020

Okay so I've compared "inflate-check baseline incremental" between the two PRs:

That's over 40x smaller, and I suspect the 0.3% "slowdown" is just noise.

While the instructions:u view doesn't fully reflect that, it's possible the remaining difference in instructions:u is noise, but I'm not ready to bet on it.

@eddyb eddyb mentioned this pull request Mar 22, 2020
Merged
@nnethercote
Copy link
Contributor

nnethercote commented Mar 22, 2020

The perf results look good:

  • Instruction counts: A couple of small regressions, but many small improvements.
  • Cycles (less reliable than instruction counts, but still reasonable): slightly larger improvements.
  • Wall-time (highly unreliable): also shows improvements, and given that instruction counts and cycles are also down, I can believe it's true or close to true.

Overall, looks like a slight performance improvement.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 23, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 23, 2020

📌 Commit 78c178b has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
@Centril
Rollup merge of rust-lang#70213 - eddyb:stalled-on-ty-or-const, r=nik…
bd725cf 
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
@Centril Centril mentioned this pull request Mar 24, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
@Centril
Rollup merge of rust-lang#70213 - eddyb:stalled-on-ty-or-const, r=nik…
a62fd3a 
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
@Centril Centril mentioned this pull request Mar 24, 2020
Closed
@Centril Centril mentioned this pull request Mar 24, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2020
@bors
Auto merge of rust-lang#70351 - Centril:rollup-tveoq3w, r=Centril
9d0ae58 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#68884 (Make the `type_of` return a generic type for generators)
 - rust-lang#69788 (Fix sequence of Type and Trait in optin-builtin-traits in Unstable Book)
 - rust-lang#70074 (Expand: nix all fatal errors)
 - rust-lang#70077 (Store idents for `DefPathData` into crate metadata)
 - rust-lang#70213 (traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).)
 - rust-lang#70259 (Use Reveal::All in MIR optimizations)
 - rust-lang#70284 (correctly handle const params in type_of)
 - rust-lang#70289 (Refactor `codegen`)

Failed merges:

r? @ghost
@bors bors merged commit 7bd86ce into rust-lang:master Mar 24, 2020
@eddyb eddyb deleted the stalled-on-ty-or-const branch March 24, 2020 17:28
@lcnr lcnr mentioned this pull request May 9, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 25, 2020
@Dylan-DPC
Rollup merge of rust-lang#72061 - lcnr:const-inference-test, r=eddyb
04daee2 
add regression tests for stalled_on const vars

closes rust-lang#70180

Afaict this has been fixed sometime after rust-lang#70213

`trait_ref_type_vars` correctly adds const infers and I did not find any remaining `FIXME`s which correspond to this issue.
https://github.com/rust-lang/rust/blob/7c59a81a5fcbaaca311f744cd7c68d99bfbb05d3/src/librustc_trait_selection/traits/fulfill.rs#L555-L557

Added both examples from the issue as regression tests and renamed `trait_ref_type_vars` -> `trait_ref_infer_vars`.

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 25, 2020
@Dylan-DPC
Rollup merge of rust-lang#72061 - lcnr:const-inference-test, r=eddyb
b3f1b95 
add regression tests for stalled_on const vars

closes rust-lang#70180

Afaict this has been fixed sometime after rust-lang#70213

`trait_ref_type_vars` correctly adds const infers and I did not find any remaining `FIXME`s which correspond to this issue.
https://github.com/rust-lang/rust/blob/7c59a81a5fcbaaca311f744cd7c68d99bfbb05d3/src/librustc_trait_selection/traits/fulfill.rs#L555-L557

Added both examples from the issue as regression tests and renamed `trait_ref_type_vars` -> `trait_ref_infer_vars`.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakis nikomatsakis

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@eddyb @rust-timer @bors @nnethercote @nikomatsakis @rust-highfive