Restrict `From<S>` for `{D,Subd}iagnosticMessage`. #110579

nnethercote · 2023-04-20T05:27:00Z

Currently a {D,Subd}iagnosticMessage can be created from any type that impls Into<String>. That includes &str, String, and Cow<'static, str>, which are reasonable. It also includes &String, which is pretty weird, and results in many places making unnecessary allocations for patterns like this:

self.fatal(&format!(...))

This creates a string with format!, takes a reference, passes the reference to fatal, which does an into(), which clones the reference, doing a second allocation. Two allocations for a single string, bleh.

This commit changes the From impls so that you can only create a {D,Subd}iagnosticMessage from &str, String, or Cow<'static, str>. This requires changing all the places that currently create one from a &String. Most of these are of the &format!(...) form described above; each one removes an unnecessary static &, plus an allocation when executed. There are also a few places where the existing use of &String was more reasonable; these now just use clone() at the call site.

As well as making the code nicer and more efficient, this is a step towards possibly using Cow<'static, str> in
{D,Subd}iagnosticMessage::{Str,Eager}. That would require changing the From<&'a str> impls to From<&'static str>, which is doable, but I'm not yet sure if it's worthwhile.

r? @davidtwco

rustbot · 2023-04-20T05:27:10Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

nnethercote · 2023-04-20T05:27:39Z

Apologies for the extremely tedious review!

@bors p=1

because it's highly conflict-prone.

nnethercote · 2023-04-20T05:28:03Z

@bors rollup=never

nnethercote · 2023-04-20T05:29:22Z

@bors try @rust-timer queue

bors · 2023-04-20T05:29:32Z

⌛ Trying commit e98557dea040a0fd347c610b5e08e812375b1a62 with merge 8a10aae2e708dc03c92bc0a12e63e93b5ee85f6b...

Ezrashaw · 2023-04-20T05:30:30Z

I suspect that the perf impact here will be smallish; LLVM should be able to figure this out in many cases. Definitely still a good change though.

Ezrashaw · 2023-04-20T05:34:21Z

src/tools/miri/src/diagnostics.rs

@@ -401,30 +401,32 @@ pub fn report_msg<'tcx>(
    } else {
        // Make sure we show the message even when it is a dummy span.
        for line in span_msg {
-            err.note(&line);


I think that these changes are best addressed in PRs on their respective repos. Clippy and Miri aren't developed in-tree, they're just pulled regularly.

You sure about that? I've done many PRs where I changed clippy and miri and rustfmt because of API changes to compiler code, and I'm following the same process here.

Oh right, this isn't just fixing a lint; there's an API change involved. My mistake sorry.

Yeah, it's definitely courteous to open the PR to them first if you can be arsed but the reason we switched to subtrees is precisely so that the development flow can be two-way (which it can't be with a submodule).

Yeah, this is a case where I think modifying in r-l/rust is fine.

JakobDegen · 2023-04-20T05:37:38Z

@Ezrashaw unless everything gets inlined, I expect LLVM to do this optimization ~never, since it's exceedingly hard to get right

Ezrashaw · 2023-04-20T05:39:12Z

@JakobDegen ~~I don't know much about LLVM optimizations but those format! calls would be inlined, right?~~ Correction, I think diagnostic calls will often get inlined?

workingjubilee · 2023-04-20T05:40:12Z

Yes, I wouldn't argue "LLVM can do this" as a reason for why not. I would expect the allocation and memcpy to be fast enough it's not worth shedding an allocation here and there, often.

BUT I have seen smaller PRs shedding fewer (source-level) allocations have pretty unequivocal wins, so I expect this one to do pretty good.

Ezrashaw · 2023-04-20T05:43:37Z

BUT I have seen smaller PRs shedding fewer (source-level) allocations have pretty unequivocal wins, so I expect this one to do pretty good.

Fair enough. This is definitely a good API change anyways, it enforces correctness :)

JakobDegen · 2023-04-20T05:45:07Z

@Ezrashaw LLVM would need to inline the format! machinery, the diagnostics call, and the clone on the other side. I doubt that's the case

nnethercote · 2023-04-20T05:51:21Z

I measured check full builds locally and the best result was a 0.48% instruction count reduction for match-stress. I didn't expect much perf effect because error message paths aren't usually hot. The PR is more about code cleanliness.

nnethercote · 2023-04-20T05:52:16Z

@bors try @rust-timer queue

bors · 2023-04-20T05:52:27Z

⌛ Trying commit c89e467512aa9844afb48998df42baf16e814d9c with merge c5e11dd12ff8c69bfcf77a98131b1660f304d1fd...

oli-obk · 2023-04-20T06:55:16Z

I'm not sure this PR is worth doing considering we're trying to move all the diagnostics to fluent/struct based ones, so hopefully we won't have any more custom .note or similar calls in the compiler at all.

That said, I gave it a review and it's mechanical and trivial enough to just do.

davidtwco · 2023-05-02T10:25:51Z

Apologies, didn't get to this quick enough, r=me once you've rebased - most of the changes will be trivial.

rustbot · 2023-05-02T22:13:43Z

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

nnethercote · 2023-05-02T22:16:27Z

@bors r=davidtwco

bors · 2023-05-02T22:16:29Z

📌 Commit bbc457ef0e3bb9a07313f7d7b9f80800a043b807 has been approved by davidtwco

It is now in the queue for this repository.

Currently a `{D,Subd}iagnosticMessage` can be created from any type that impls `Into<String>`. That includes `&str`, `String`, and `Cow<'static, str>`, which are reasonable. It also includes `&String`, which is pretty weird, and results in many places making unnecessary allocations for patterns like this: ``` self.fatal(&format!(...)) ``` This creates a string with `format!`, takes a reference, passes the reference to `fatal`, which does an `into()`, which clones the reference, doing a second allocation. Two allocations for a single string, bleh. This commit changes the `From` impls so that you can only create a `{D,Subd}iagnosticMessage` from `&str`, `String`, or `Cow<'static, str>`. This requires changing all the places that currently create one from a `&String`. Most of these are of the `&format!(...)` form described above; each one removes an unnecessary static `&`, plus an allocation when executed. There are also a few places where the existing use of `&String` was more reasonable; these now just use `clone()` at the call site. As well as making the code nicer and more efficient, this is a step towards possibly using `Cow<'static, str>` in `{D,Subd}iagnosticMessage::{Str,Eager}`. That would require changing the `From<&'a str>` impls to `From<&'static str>`, which is doable, but I'm not yet sure if it's worthwhile.

nnethercote · 2023-05-02T22:46:15Z

@bors r=davidtwco

bors · 2023-05-02T22:46:16Z

📌 Commit 6b62f37 has been approved by davidtwco

It is now in the queue for this repository.

bors · 2023-05-03T00:22:24Z

⌛ Testing commit 6b62f37 with merge 71af5c4...

bors · 2023-05-03T03:38:44Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 71af5c4 to master...

rust-timer · 2023-05-03T04:56:15Z

Finished benchmarking commit (71af5c4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.4%]	3
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.5%]	5
All ❌✅ (primary)	-0.5%	[-0.5%, -0.4%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	3.8%	[3.8%, 3.8%]	1

Cycles

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

Binary size

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

Bootstrap: 657.453s -> 654.674s (-0.42%)

rustbot assigned davidtwco Apr 20, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 20, 2023

This comment has been minimized.

Sign in to view

Ezrashaw reviewed Apr 20, 2023

View reviewed changes

nnethercote force-pushed the restrict-From-for-Diagnostics branch from e98557d to c89e467 Compare April 20, 2023 05:51

This comment has been minimized.

Sign in to view

nnethercote force-pushed the restrict-From-for-Diagnostics branch from c29b6d3 to bbc457e Compare May 2, 2023 22:13

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 2, 2023

This comment has been minimized.

Sign in to view

nnethercote force-pushed the restrict-From-for-Diagnostics branch from bbc457e to 6b62f37 Compare May 2, 2023 22:45

bors added the merged-by-bors This PR was explicitly merged by bors. label May 3, 2023

bors merged commit 71af5c4 into rust-lang:master May 3, 2023

rustbot added this to the 1.71.0 milestone May 3, 2023

nnethercote deleted the restrict-From-for-Diagnostics branch May 3, 2023 06:23

xFrednet mentioned this pull request May 3, 2023

Bump nightly -> 2023-06-01 rust-marker/marker#128

Closed

nnethercote mentioned this pull request May 16, 2023

Avoid &format("...") calls in error message code. #111633

Merged

matthiaskrgr mentioned this pull request Aug 25, 2023

3d vector compiler bug #115203

Closed

matthiaskrgr mentioned this pull request Sep 3, 2023

ICE: unexpected bound var resolution for HirId(DefId(..)) #115497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict `From<S>` for `{D,Subd}iagnosticMessage`. #110579

Restrict `From<S>` for `{D,Subd}iagnosticMessage`. #110579

nnethercote commented Apr 20, 2023 •

edited

Loading

rustbot commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

This comment has been minimized.

bors commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023

This comment has been minimized.

Ezrashaw Apr 20, 2023

nnethercote Apr 20, 2023

Ezrashaw Apr 20, 2023

workingjubilee Apr 20, 2023

compiler-errors Apr 20, 2023

JakobDegen commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023 •

edited

Loading

workingjubilee commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023

JakobDegen commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

This comment has been minimized.

bors commented Apr 20, 2023

This comment has been minimized.

oli-obk commented Apr 20, 2023

davidtwco commented May 2, 2023

rustbot commented May 2, 2023

nnethercote commented May 2, 2023

bors commented May 2, 2023

This comment has been minimized.

nnethercote commented May 2, 2023

bors commented May 2, 2023

bors commented May 3, 2023

bors commented May 3, 2023

rust-timer commented May 3, 2023

Restrict From<S> for {D,Subd}iagnosticMessage. #110579

Restrict From<S> for {D,Subd}iagnosticMessage. #110579

Conversation

nnethercote commented Apr 20, 2023 • edited Loading

rustbot commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

This comment has been minimized.

bors commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023 • edited Loading

workingjubilee commented Apr 20, 2023

Ezrashaw commented Apr 20, 2023

JakobDegen commented Apr 20, 2023

nnethercote commented Apr 20, 2023

nnethercote commented Apr 20, 2023

This comment has been minimized.

bors commented Apr 20, 2023

This comment has been minimized.

oli-obk commented Apr 20, 2023

davidtwco commented May 2, 2023

rustbot commented May 2, 2023

nnethercote commented May 2, 2023

bors commented May 2, 2023

This comment has been minimized.

nnethercote commented May 2, 2023

bors commented May 2, 2023

bors commented May 3, 2023

bors commented May 3, 2023

rust-timer commented May 3, 2023

Overall result: ✅ improvements - no action needed

Restrict `From<S>` for `{D,Subd}iagnosticMessage`. #110579

Restrict `From<S>` for `{D,Subd}iagnosticMessage`. #110579

nnethercote commented Apr 20, 2023 •

edited

Loading

Ezrashaw commented Apr 20, 2023 •

edited

Loading