Skip to content

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 13, 2022

the existing TreatParams enum pretty much mixes everything up. Not sure why this looked right to me in #94057

This also includes two changes which impact perf:

  • ty::Projection with inference vars shouldn't be treated as a rigid type, even if fully normalized
  • ty::Placeholder only unifies with itself, so actually return Some for them

r? @nikomatsakis

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2022
@lcnr lcnr force-pushed the simplify_type-sus branch 3 times, most recently from f83cc93 to 6ebef2b Compare May 13, 2022 18:38
@lcnr
Copy link
Contributor Author

lcnr commented May 13, 2022

I feel like SimplifiedType isn't a good name here. Maybe something like RigidTyConstructor or something like that would be better 🤔

edit: after talking to @BoxyUwU, InjectiveTyConstructor might be a pretty good name

@lcnr lcnr force-pushed the simplify_type-sus branch 2 times, most recently from b21e83d to ff823ee Compare May 13, 2022 18:48
@lcnr lcnr force-pushed the simplify_type-sus branch from ff823ee to db19e2b Compare May 18, 2022 07:00
@lcnr
Copy link
Contributor Author

lcnr commented May 19, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 19, 2022
@bors
Copy link
Collaborator

bors commented May 19, 2022

⌛ Trying commit db19e2b with merge c067287...

/// it may still succeed later if the projection contains any inference
/// variables.
AsPlaceholder,
AsInfer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, I am equally to blame :) it's really just universal (placeholders) vs existential (infer)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 19, 2022

📌 Commit db19e2b 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 May 19, 2022
@bors
Copy link
Collaborator

bors commented May 19, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing c067287 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2022
@bors bors merged commit c067287 into rust-lang:master May 19, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 19, 2022
@lcnr lcnr deleted the simplify_type-sus branch May 19, 2022 14:48
@lcnr
Copy link
Contributor Author

lcnr commented May 19, 2022

oh, we've used the @bors try from before the r+ to actually merge this PR? Idk if that's an issue? 🤔 cc @rust-lang/infra

@Mark-Simulacrum
Copy link
Member

Gah. rust-lang/homu#47 is the issue for this, we'll need to back this out from master I think, I'm not sure whether a force push dropping it is the best idea.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c067287): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 0 0 3 0
mean2 N/A N/A N/A -1.6% N/A
max N/A N/A N/A -1.8% N/A

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 2 3 1 3
mean2 N/A 1.7% -1.4% -2.2% -1.4%
max N/A 2.0% -1.4% -2.2% -1.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

@Mark-Simulacrum
Copy link
Member

To follow up on the merge -- we manually ran CI on the merged commit, which thankfully passed. See https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/try-based.20merge.20into.20master for some discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-perf Status: Waiting on a perf run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants