Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a leak in DiagnosticBuilder::into_diagnostic. #69628

Merged
merged 1 commit into from
Mar 3, 2020
Merged

Fix a leak in DiagnosticBuilder::into_diagnostic. #69628

merged 1 commit into from
Mar 3, 2020

Conversation

nnethercote
Copy link
Contributor

Fixes #69600.

r? @Centril

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

I did a local perf run and the change had absolutely no effect on perf, as you'd hope. I suspect the code in question isn't even run unless an error is issued, which is relatively rare.

@Centril
Copy link
Contributor

Centril commented Mar 2, 2020

Pretty unfortunate that we cannot just do let DiagnosticBuilderInner { handler, diagnostic, .. } = *self.0; -- one wonders whether that language restriction causes more harm than good...

r=me when #69628 (comment) has resolved itself.

@nnethercote nnethercote force-pushed the fix-DiagnosticBuilder-into_diagnostic-leak branch from 83b38a5 to 99a595e Compare March 2, 2020 03:43
@Centril
Copy link
Contributor

Centril commented Mar 2, 2020 •
edited
Loading

@nnethercote btw, Diagnostic::new will always allocate due to message: vec![(message.to_owned(), Style::NoStyle)],, so if you are concerned about that, which you could reasonably not be, then a struct literal might be more efficient.

@nnethercote
Copy link
Contributor Author

Thanks for the tip! The code is cold enough that the additional allocation doesn't matter, I think. But let's double-check that...

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 2, 2020

⌛ Trying commit 99a595e with merge 017a682...

bors added a commit that referenced this pull request Mar 2, 2020
@bors
Auto merge of #69628 - nnethercote:fix-DiagnosticBuilder-into_diagnos…
017a682 
…tic-leak, r=<try>

Fix a leak in `DiagnosticBuilder::into_diagnostic`.

Fixes #69600.

r? @Centril
@bors
Copy link
Contributor

bors commented Mar 2, 2020

☀️ Try build successful - checks-azure
Build commit: 017a682 (017a6824e7474a9baad3b2bbdff3dd5f27a00fe5)

@rust-timer
Copy link
Collaborator

Queued 017a682 with parent 9dc8dad, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 017a682, comparison URL.

@nnethercote
Copy link
Contributor Author

Yep, the perf impact is just noise.

@bors r=Centril
@bors rollup=always

@bors
Copy link
Contributor

bors commented Mar 2, 2020

📌 Commit 99a595e has been approved by Centril

@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 2, 2020
@nnethercote
Copy link
Contributor Author

Should this be backported onto beta?

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 3, 2020
@Dylan-DPC
Rollup merge of rust-lang#69628 - nnethercote:fix-DiagnosticBuilder-i…
6a07f16 
…nto_diagnostic-leak, r=Centril

Fix a leak in `DiagnosticBuilder::into_diagnostic`.

Fixes rust-lang#69600.

r? @Centril
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 3, 2020
Closed
bors added a commit that referenced this pull request Mar 3, 2020
@bors
Auto merge of #69652 - Dylan-DPC:rollup-0mlvtin, r=Dylan-DPC
bf320fe 
Rollup of 9 pull requests

Successful merges:

 - #69565 (miri engine: turn some debug_assert into assert)
 - #69609 (Remove `usable_size` APIs)
 - #69620 (doc(librustc_error_codes): add long error explanation for E0719)
 - #69626 (Toolstate: don't duplicate nightly tool list.)
 - #69628 (Fix a leak in `DiagnosticBuilder::into_diagnostic`.)
 - #69633 (Update my mailmap entry)
 - #69634 (clean up E0378 explanation)
 - #69637 (Don't convert Results to Options just for matching.)
 - #69641 (Update books)

Failed merges:

r? @ghost
@JohnTitor JohnTitor mentioned this pull request Mar 3, 2020
Merged
bors added a commit that referenced this pull request Mar 3, 2020
@bors
Auto merge of #69666 - JohnTitor:rollup-6nt3op0, r=JohnTitor
a5de254 
Rollup of 9 pull requests

Successful merges:

 - #69213 (Improve documentation on iterators length)
 - #69609 (Remove `usable_size` APIs)
 - #69619 (more cleanups)
 - #69620 (doc(librustc_error_codes): add long error explanation for E0719)
 - #69626 (Toolstate: don't duplicate nightly tool list.)
 - #69628 (Fix a leak in `DiagnosticBuilder::into_diagnostic`.)
 - #69633 (Update my mailmap entry)
 - #69634 (clean up E0378 explanation)
 - #69637 (Don't convert Results to Options just for matching.)

Failed merges:

r? @ghost
@bors bors merged commit ef311d5 into rust-lang:master Mar 3, 2020
@nnethercote nnethercote deleted the fix-DiagnosticBuilder-into_diagnostic-leak branch March 3, 2020 23:56
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 4, 2020
@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 4, 2020
@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2020

beta-accepted

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 5, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 5, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 5, 2020
Merged
bors added a commit that referenced this pull request Mar 6, 2020
@bors
Auto merge of #69748 - Mark-Simulacrum:beta-next, r=Mark-Simulacrum
b08d071 
[beta] another round of backports for 1.42

This backports the following pull requests:

* Fix a leak in `DiagnosticBuilder::into_diagnostic`. #69628
* stash API: remove panic to fix ICE. #69623
* Do not ICE on invalid type node after parse recovery #69583
* Backport only: avoid ICE on bad placeholder type #69324
* add regression test for issue #68794 #69168
* Update compiler-builtins to 0.1.25 #69086
* Update RELEASES.md for 1.42.0 #68989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwrensha dwrensha dwrensha left review comments

@Centril Centril Centril left review comments

Assignees

@Centril Centril

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

memory leak in DiagnosticBuilder.into_diagnostic()
8 participants
@nnethercote @Centril @rust-timer @bors @pnkfelix @dwrensha @Mark-Simulacrum @rust-highfive