Revert 133858 due to perf regression #135263
Conversation
rustbot
added
S-waiting-on-review
|
Ok, this is going to miss the nightly cutoff but it's fine. |
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
fml |
|
As long as this doesnt miss the beta cut off in like a month its technically fine right? It's "only" a 20% regression to some crates on nightly |
|
The revert doesn't apply cleanly since #134523 landed in the meantime. I've rebased to fix the conflicts, hopefully correctly to change the new code from #134523 to match the pre-revert state -- and #133858 landing also created conflicts for #134523. It's conflict city over here. That will make #134523's beta backport harder...
Maybe not fine per se, but in a sense yes, and in another sense less so: if another perf run had been done pre-merge, the PR likely wouldn't have landed yet. A revert was requested and we usually are revert-happy: it's usually easier to investigate issues without any additional time pressure on the author. |
|
Ah right... merge conflicts |
|
This code should generally be slow-moving, but yes it'd be nice if not too much code started relying on #133858. |
| @@ -1207,7 +1206,15 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> { | |||
| borrowed, | |||
| |diag| { | |||
| session_diagnostics::TailExprDropOrder { borrowed }.decorate_lint(diag); | |||
| explain.add_explanation_to_diagnostic(&this, diag, "", None, None); | |||
| explain.add_explanation_to_diagnostic( | |||
| this.infcx.tcx, | |||
|
@bors r+ rollup=never |
bors
added
S-waiting-on-bors
|
Perf issue looks resolved #135273 (comment), still want to revert? |
|
@bors r- Let's not cause churn if a more targeted fix is still being evaluated |
bors
added
S-waiting-on-author
|
After discussing in today's t-compiler meeting, we chose to land the perf fix with the small diagnostics regression rather than this revert. |
|
In the future if you're unsure whether you would actually like to revert or merge a fix can you keep the PR marked as draft, I was also under the impression there was a level of "urgency" here as you seemed to even want this to make it into the nightly cut off. From my POV it seemed clear that you wanted this merged and you wanted it merged quickly to avoid merge conflicts. On the other hand reading the meeting thread I am not sure if that is actually accurate as you seemed to want opinions from others on T-compiler and it also did not seem to take very much discussion to be convinced that merging the fix was the better move It's not a big deal I'm just slightly confused as to what happend here as it seems like there was some kind of miscommunication 😅 |
|
Ah I wasn’t unsure. I wanted this merged quickly indeed, and also disagree with the choice that was made 😓. I marked this ready yesterday when the alternative solution didn’t exist then, nor did we know whether there’d be one. I asked in the meeting because lcnr had no opinion on the alternative PR, which is fine of course. I didn’t want to discuss it more in the meeting, because there were now two solutions in flight, decreasing the urgency, and I had already spent a lot of time on this issue that only seemed important to me and moved on. In any case, I’ll keep your comment in mind in the future. Sorry, if this caused you any inconvenience. |
|
Ah okay that makes sense, no worries then. It sounds like I did understand things correctly and it was just unfortunate timing on me getting around to reviewing this and the compiler meeting happening. |
|
In the future I'll also make sure to say something in the revert PR if I open an alternative. Sorry for the trouble, and thanks for handling all this! |
This PR prepares a revert of #133858 due to the unexpected perf regressions.
cc PR author @dianne and reviewer @lcnr
Opening as draft to leave some time for @dianne to take a look in case the fix is obvious. If not, or if they don't have time to look into it soon, we can land this revert. Once the issue is fixed in the original PR we can reland it with ease.