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

Use Symbol more #60630

Merged
merged 5 commits into from
May 13, 2019
Merged

Use Symbol more #60630

merged 5 commits into from
May 13, 2019

Conversation

nnethercote
Copy link
Contributor

A Symbol can be equated with a string (e.g. &str). This involves a
TLS lookup to get the chars (and a Mutex lock in a parallel compiler)
and then a char-by-char comparison. This functionality is convenient but
avoids one of the main benefits of Symbols, which is fast equality
comparisons.

This PR removes the Symbol/string equality operations, forcing a lot
of existing string occurrences to become Symbols. Fortunately, these
are almost all static strings (many are attribute names) and we can add
static Symbols as necessary, and very little extra interning occurs.
The benefits are (a) a slight speedup (possibly greater in a parallel
compiler), and (b) the code is a lot more principled about Symbol use.
The main downside is verbosity, particularly with more use syntax::symbol::symbols items.

r? @Zoxc

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2019
@nnethercote nnethercote changed the title Use symbol more Use Symbol more May 8, 2019
@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented May 8, 2019

⌛ Trying commit b28d0aeaa86dd5079501c37d439b60771c34b2e8 with merge 1a89a3215ec97a7b0b2a010b773e7b8be90f6f4e...

@petrochenkov petrochenkov self-assigned this May 8, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2019

bors seems to have gotten stuck

@bors try

@bors
Copy link
Contributor

bors commented May 8, 2019

💥 Test timed out

@mati865
Copy link
Contributor

mati865 commented May 8, 2019

@oli-obk I think you have to use retry, commit hash hasn't changed.

@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2019

@bors retry

@bors
Copy link
Contributor

bors commented May 8, 2019

⌛ Trying commit b28d0aeaa86dd5079501c37d439b60771c34b2e8 with merge ce805fd53ba59be4d9bd0f0987f1e5b764831307...

@bors
Copy link
Contributor

bors commented May 8, 2019

☔ The latest upstream changes (presumably #60246) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented May 8, 2019

☀️ Try build successful - checks-travis
Build commit: ce805fd53ba59be4d9bd0f0987f1e5b764831307

@Zoxc
Copy link
Contributor

Zoxc commented May 8, 2019

@rust-timer build ce805fd53ba59be4d9bd0f0987f1e5b764831307

@rust-timer
Copy link
Collaborator

Success: Queued ce805fd53ba59be4d9bd0f0987f1e5b764831307 with parent b92d360, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ce805fd53ba59be4d9bd0f0987f1e5b764831307

@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2019

I think github is being bugged by time travellers today

@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2019

One 1.4% perf improvement, many small ones (<1%)

@eddyb
Copy link
Member

eddyb commented May 8, 2019

cc @petrochenkov

(I haven't done a full review but this does sound like something I've been wanting for a long time)

@eddyb
Copy link
Member

eddyb commented May 8, 2019

cc @petrochenkov

@nnethercote
Copy link
Contributor Author

The symbols:: qualifier is now common enough that changing it to syms:: might be worth considering.

}

// Non-identifer symbols that can be referred to with syntax_pos::nisymbols::*.
NISymbols {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Non-identness" doesnt' really matter, entries in the Symbols { ... } block should probably just allow an optional : "non-ident" literal (with everything generated in one mod symbols).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realize that was possible. Good suggestion, I will implement it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like us to have 2 namespaces here, so if you use symbols::a you know you're getting a and not some other identifier. It also ensure there won't be any name conflicts if you add a: "b" and you want to add a later.

We could probably rename symbols to idents in that case.

@petrochenkov
Copy link
Contributor

So, my primary concern is that we are front-loading interning of about 500 strings and do it in every compilation session even if most of them won't be actually used.

Could you measure how much time the UI test suite (a lot of little programs) takes with and without these changes?
If the change is negligible, then creation of the pre-interned table is cheap enough to be front-loaded safely.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2019
@bors
Copy link
Contributor

bors commented May 13, 2019

⌛ Testing commit ea9fac5 with merge fe5f42c...

bors added a commit that referenced this pull request May 13, 2019
Use `Symbol` more

A `Symbol` can be equated with a string (e.g. `&str`). This involves a
TLS lookup to get the chars (and a Mutex lock in a parallel compiler)
and then a char-by-char comparison. This functionality is convenient but
avoids one of the main benefits of `Symbol`s, which is fast equality
comparisons.

This PR removes the `Symbol`/string equality operations, forcing a lot
of existing string occurrences to become `Symbol`s. Fortunately, these
are almost all static strings (many are attribute names) and we can add
static `Symbol`s as necessary, and very little extra interning occurs.
The benefits are (a) a slight speedup (possibly greater in a parallel
compiler), and (b) the code is a lot more principled about `Symbol` use.
The main downside is verbosity, particularly with more `use
syntax::symbol::symbols` items.

r? @Zoxc
@bors
Copy link
Contributor

bors commented May 13, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing fe5f42c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2019
@bors bors merged commit ea9fac5 into rust-lang:master May 13, 2019
@nnethercote nnethercote deleted the use-Symbol-more branch May 13, 2019 04:50
@nnethercote nnethercote mentioned this pull request May 13, 2019
bors added a commit that referenced this pull request May 14, 2019
Simplify use of keyword symbols

They mirror non-keyword symbols now (see #60630).

`keywords::MyKeyword.name()` -> `kw::MyKeyword`
`keywords::MyKeyword.ident()` -> `Ident::with_empty_ctxt(kw::MyKeyword)` (not common)
`keywords::Invalid.ident()` -> `Ident::invalid()` (more common)

Keywords are simply `Symbol` constants now, the `Keyword` struct is eliminated.
This means `kw::MyKeyword` can now be used in `match` in particular.
nnethercote added a commit to nnethercote/rust that referenced this pull request May 21, 2019
`Symbol` received the same treatment in rust-lang#60630.

Also, we can derive `PartialEq` for `InternedString`.
bors added a commit that referenced this pull request May 21, 2019
… r=<try>

Remove impls for `InternedString`/string equality.

`Symbol` received the same treatment in #60630.

Also, we can derive `PartialEq` for `InternedString`.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
…alEq-impls, r=petrochenkov

Remove impls for `InternedString`/string equality.

`Symbol` received the same treatment in rust-lang#60630.

Also, we can derive `PartialEq` for `InternedString`.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
…alEq-impls, r=petrochenkov

Remove impls for `InternedString`/string equality.

`Symbol` received the same treatment in rust-lang#60630.

Also, we can derive `PartialEq` for `InternedString`.

r? @petrochenkov
bors added a commit that referenced this pull request May 23, 2019
Simplify use of keyword symbols

They mirror non-keyword symbols now (see #60630).

`keywords::MyKeyword.name()` -> `kw::MyKeyword`
`keywords::MyKeyword.ident()` -> `Ident::with_empty_ctxt(kw::MyKeyword)` (not common)
`keywords::Invalid.ident()` -> `Ident::invalid()` (more common)

Keywords are simply `Symbol` constants now, the `Keyword` struct is eliminated.
This means `kw::MyKeyword` can now be used in `match` in particular.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.