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.

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

Fix a leak in `DiagnosticBuilder::into_diagnostic`. #69628

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Mar 2, 2020

Fixes #69600.

r? @Centril

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Mar 2, 2020

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

This comment has been minimized.

Copy link
Member

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 nnethercote:fix-DiagnosticBuilder-into_diagnostic-leak branch from 83b38a5 to 99a595e Mar 2, 2020
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Mar 2, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Mar 2, 2020

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

This comment has been minimized.

Copy link

rust-timer commented Mar 2, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

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
…tic-leak, r=<try>

Fix a leak in `DiagnosticBuilder::into_diagnostic`.

Fixes #69600.

r? @Centril
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 2, 2020

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Mar 2, 2020

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Mar 2, 2020

Finished benchmarking try commit 017a682, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Mar 2, 2020

Yep, the perf impact is just noise.

@bors r=Centril
@bors rollup=always

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 2, 2020

📌 Commit 99a595e has been approved by Centril

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Mar 2, 2020

Should this be backported onto beta?

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 3, 2020
…nto_diagnostic-leak, r=Centril

Fix a leak in `DiagnosticBuilder::into_diagnostic`.

Fixes rust-lang#69600.

r? @Centril
bors added a commit that referenced this pull request Mar 3, 2020
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
bors added a commit that referenced this pull request Mar 3, 2020
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
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200302.8 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:fix-DiagnosticBuilder-into_diagnostic-leak branch Mar 3, 2020
@Centril Centril added the T-compiler label Mar 4, 2020
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 5, 2020

beta-accepted

bors added a commit that referenced this pull request Mar 6, 2020
[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
bors added a commit that referenced this pull request Mar 6, 2020
[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
bors added a commit that referenced this pull request Mar 6, 2020
[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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.