Remove `SymbolStr` #91957

nnethercote · 2021-12-15T06:36:12Z

This was originally proposed in #74554 (comment). As well as removing the icky SymbolStr type, it allows the removal of a lot of & and * occurrences.

Best reviewed one commit at a time.

r? @oli-obk

By changing `as_str()` to take `&self` instead of `self`, we can just return `&str`. We're still lying about lifetimes, but it's a smaller lie than before, where `SymbolStr` contained a (fake) `&'static str`!

rust-highfive · 2021-12-15T06:36:14Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

Some changes occurred in clean/types.rs.

cc @camelid

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in intra-doc-links.

cc @jyn514

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occured to rustc_codegen_gcc

cc @antoyo

rust-highfive · 2021-12-15T06:36:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

Please see the contribution instructions for more information.

nnethercote · 2021-12-15T06:37:23Z

compiler/rustc_span/src/symbol.rs

+    /// as `&self`, but actually tied to the lifetime of the underlying
+    /// interner. Interners are long-lived, and there are very few of them, and
+    /// this function is typically used for short-lived things, so in practice
+    /// it works out ok.


I would be happy to see a better description than this, if anyone has one to offer.

oli-obk · 2021-12-15T14:23:01Z

@bors try @rust-timer queue

rust-timer · 2021-12-15T14:23:02Z

Awaiting bors try build completion.

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

bors · 2021-12-15T14:23:11Z

⌛ Trying commit b1c934e with merge 2b0871127a9ec9258077c392af12778176772fa1...

bors · 2021-12-15T16:17:44Z

☀️ Try build successful - checks-actions
Build commit: 2b0871127a9ec9258077c392af12778176772fa1 (2b0871127a9ec9258077c392af12778176772fa1)

rust-timer · 2021-12-15T16:17:46Z

Queued 2b0871127a9ec9258077c392af12778176772fa1 with parent 3ee016a, future comparison URL.

compiler/rustc_span/src/symbol.rs

camelid · 2021-12-15T17:19:25Z

compiler/rustc_span/src/symbol.rs

+    /// Note that the lifetime of the return value is a lie. It's not the same
+    /// as `&self`, but actually tied to the lifetime of the underlying
+    /// interner. Interners are long-lived, and there are very few of them, and
+    /// this function is typically used for short-lived things, so in practice
+    /// it works out ok.


I made a few edits. Feel free to apply none, some, or all:

Suggested change

/// Note that the lifetime of the return value is a lie. It's not the same

/// as `&self`, but actually tied to the lifetime of the underlying

/// interner. Interners are long-lived, and there are very few of them, and

/// this function is typically used for short-lived things, so in practice

/// it works out ok.

/// Note that the lifetime of the return value is a lie, so this function

/// is unsound. The runtime lifetime is not the same as `&self`;

/// it's actually tied to the lifetime of the underlying interner.

/// Interners are long-lived, there are very few of them, and

/// this function is typically used for short-lived things; so in practice

/// it works out ok.

rust-timer · 2021-12-15T17:49:17Z

Finished benchmarking commit (2b0871127a9ec9258077c392af12778176772fa1): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -5.7% on incr-unchanged builds of inflate)
Small regression in instruction counts (up to 0.8% on incr-patched: println builds of coercions)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

RalfJung · 2021-12-15T18:01:52Z

compiler/rustc_span/src/symbol.rs

+    /// requires locking the symbol interner.
+    ///
+    /// Note that the lifetime of the return value is a lie. See
+    /// `Symbol::as_str()` for details.


Uh, do we really want to have unsound functions in widely used crate-public APIs?

I very strongly feel that the answer is "no"...

FWIW, the API was already unsound before this, the comment just makes it slightly more obvious.

OTOH If anything this allows less code than the previous API (since the old return type outlived 'static), so I am confused why new warnings about unsoundness are coming up now.

Ah I found #91957 (comment), it was collapsed.

well, we already had unsound functions in widely used rustc internal APIs. This PR just makes it less easy to abuse, so the PR by itself is not making the situation worse.

The only real way to fix this is to start tracking lifetimes for it everywhere, which is a much bigger change.

If we got rid of the use case of running a compiler twice in the same process we could probably just never drop the interner and then everything would be sound?

I don't get this argument. as_str now produces something that you can't really pass around at all, where SymbolStr had no lifetime binding it to Symbol. Sure you can send Symbol around, but you could do that before?

The &str is Send and Sync either way.

The &str is Send and Sync either way.

Hm yeah that is fair. One could even implement the new API based on the old one in safe code (by leaking some memory), as you showed.

I think the only potential downside of the new API is that it makes it more annoying to unsoundly use it. Before, you would have had to store all the SymbolStrs somewhere, but now you just need the Symbols to come from a long-lived source.

I think the only potential downside of the new API is that it makes it more annoying to unsoundly use it

Is that a downside? Sounds like a good thing to me 😄

nnethercote · 2021-12-15T22:33:50Z

To be entirely sound the .as_str() method would need to be replaced with a .with_str() method taking a closure.

I looked at this a while back. There are several hundred uses of Symbol::as_str() and Ident::as_str(). For a lot of use cases it would be fine because the &str is used in a single place, often with a predicate like this:

if !ident.as_str().is_ascii() { ... }

But there are plenty of uses where a local &str is obtained and then that's used in multiple places within that function, with those uses often separated by multiple lines, and rewriting things within a closure to fit a with would be painful.

jyn514 · 2021-12-15T22:41:30Z

Can we add a lifetime to Symbol that matches the lifetime of the interner? Something like fn intern(&self) -> Symbol<'_>; and all the preinterned symbols would have lifetime Symbol<'static>.

bjorn3 · 2021-12-15T23:49:27Z

I expect that to be painful as Symbol is used in various places where there is no existing context to tie a lifetime to.

jyn514 · 2021-12-16T00:59:18Z

What do you mean? Symbols can't be used before the interner is constructed or intern() will panic.

bjorn3 · 2021-12-16T08:31:05Z

Currently no context that could contain a reference to the interner is passed around at a lot of places. Instead the TLS is used to obain the interner. Adding a lifetime to Symbol will require plumbing through a reference to the interner to a lot of places.

oli-obk · 2021-12-16T19:08:55Z

@bors r+

Let's move this along, it's a strict improvement, even if it doesn't fix the unsoundness

bors · 2021-12-16T19:08:57Z

📌 Commit b1c934e has been approved by oli-obk

bors · 2021-12-19T09:31:41Z

⌛ Testing commit b1c934e with merge a41a692...

bors · 2021-12-19T12:36:50Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a41a692 to master...

rust-highfive · 2021-12-19T12:37:14Z

📣 Toolstate changed by #91957!

Tested on commit a41a692.
Direct link to PR: #91957

💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb).

@oli-obk

Tested on commit rust-lang/rust@a41a692. Direct link to PR: <rust-lang/rust#91957> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb).

rust-timer · 2021-12-19T14:07:31Z

Finished benchmarking commit (a41a692): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -0.2% on full builds of externs)
Moderate regression in instruction counts (up to 3.1% on incr-patched: println builds of regression-31157)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

camelid · 2021-12-19T19:08:44Z

regression-31157 opt seems to have been showing spurious perf changes recently. It happened to me on a rustdoc PR, where it's impossible for there to be perf changes for an opt build (since only doc builds can have changes).

camsteffen · 2021-12-19T20:05:32Z

compiler/rustc_span/src/symbol.rs

-    fn to_stable_hash_key(&self, _: &CTX) -> SymbolStr {
-        self.as_str()
+    fn to_stable_hash_key(&self, _: &CTX) -> String {
+        self.as_str().to_string()


We could return &'static str here. It would propagate the lifetime lie, but basically the same as before.

Remove `SymbolStr` This was originally proposed in rust-lang#74554 (comment). As well as removing the icky `SymbolStr` type, it allows the removal of a lot of `&` and `*` occurrences. Best reviewed one commit at a time. r? `@oli-obk`

rylev · 2021-12-21T15:29:18Z

This seems likely to be noise as a result of the issue tracked in rust-lang/rustc-perf#1126 -- marking as triaged, not something we should address directly.

@rustbot label +perf-regression-triaged

Remove `SymbolStr` This was originally proposed in rust-lang#74554 (comment). As well as removing the icky `SymbolStr` type, it allows the removal of a lot of `&` and `*` occurrences. Best reviewed one commit at a time. r? `@oli-obk`

nnethercote added 3 commits December 15, 2021 13:30

Remove SymbolStr.

8cddcd3

By changing `as_str()` to take `&self` instead of `self`, we can just return `&str`. We're still lying about lifetimes, but it's a smaller lie than before, where `SymbolStr` contained a (fake) `&'static str`!

Remove unnecessary sigils around Symbol::as_str() calls.

056d48a

Remove unnecessary sigils around Ident::as_str() calls.

b1c934e

rust-highfive assigned oli-obk Dec 15, 2021

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2021

nnethercote commented Dec 15, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 15, 2021

camelid reviewed Dec 15, 2021

View reviewed changes

compiler/rustc_span/src/symbol.rs Show resolved Hide resolved

camelid reviewed Dec 15, 2021

View reviewed changes

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 15, 2021

RalfJung reviewed Dec 15, 2021

View reviewed changes

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 Dec 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2021

bors merged commit a41a692 into rust-lang:master Dec 19, 2021

rustbot added this to the 1.59.0 milestone Dec 19, 2021

rust-highfive mentioned this pull request Dec 19, 2021

miri no longer builds after rust-lang/rust#91957 #92101

Closed

This was referenced Dec 19, 2021

rustdoc: make icons more consistent #91735

Closed

rustc_lint: Some early linting refactorings #91526

Merged

Pass Context as &mut in rustdoc #91468

Closed

Handle notable trait popup differently #91431

Closed

camsteffen reviewed Dec 19, 2021

View reviewed changes

nnethercote deleted the rm-SymbolStr branch December 20, 2021 06:14

calebcartwright mentioned this pull request Dec 21, 2021

Improve out of line module resolution rust-lang/rustfmt#5142

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 21, 2021

Kobzol mentioned this pull request Dec 28, 2021

Change Symbol::as_str to &self -> &str #83295

Closed

nnethercote mentioned this pull request Jan 28, 2022

Inlined symbols #74554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `SymbolStr` #91957

Remove `SymbolStr` #91957

nnethercote commented Dec 15, 2021

rust-highfive commented Dec 15, 2021

rust-highfive commented Dec 15, 2021

nnethercote Dec 15, 2021

oli-obk commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

camelid Dec 15, 2021 •

edited

rust-timer commented Dec 15, 2021

RalfJung Dec 15, 2021

jyn514 Dec 15, 2021

RalfJung Dec 15, 2021

RalfJung Dec 15, 2021

oli-obk Dec 15, 2021

oli-obk Dec 15, 2021

bjorn3 Dec 15, 2021

RalfJung Dec 15, 2021 •

edited

camelid Dec 15, 2021

nnethercote Dec 15, 2021

nnethercote commented Dec 15, 2021

jyn514 commented Dec 15, 2021

bjorn3 commented Dec 15, 2021

jyn514 commented Dec 16, 2021

bjorn3 commented Dec 16, 2021

oli-obk commented Dec 16, 2021

bors commented Dec 16, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

rust-highfive commented Dec 19, 2021

rust-timer commented Dec 19, 2021

camelid commented Dec 19, 2021 •

edited

camsteffen Dec 19, 2021

rylev commented Dec 21, 2021

Remove SymbolStr #91957

Remove SymbolStr #91957

Conversation

nnethercote commented Dec 15, 2021

rust-highfive commented Dec 15, 2021

rust-highfive commented Dec 15, 2021

Choose a reason for hiding this comment

oli-obk commented Dec 15, 2021

rust-timer commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

rust-timer commented Dec 15, 2021

camelid Dec 15, 2021 • edited

Choose a reason for hiding this comment

rust-timer commented Dec 15, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Dec 15, 2021

jyn514 commented Dec 15, 2021

bjorn3 commented Dec 15, 2021

jyn514 commented Dec 16, 2021

bjorn3 commented Dec 16, 2021

oli-obk commented Dec 16, 2021

bors commented Dec 16, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

rust-highfive commented Dec 19, 2021

rust-timer commented Dec 19, 2021

camelid commented Dec 19, 2021 • edited

Choose a reason for hiding this comment

rylev commented Dec 21, 2021

Remove `SymbolStr` #91957

Remove `SymbolStr` #91957

camelid Dec 15, 2021 •

edited

RalfJung Dec 15, 2021 •

edited

camelid commented Dec 19, 2021 •

edited