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

Remove `InternedString` #65657

Merged
merged 7 commits into from Oct 24, 2019
Merged

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Oct 21, 2019

This PR removes InternedString by converting all occurrences to Symbol. There are a handful of places that need to use the symbol chars instead of the symbol index, e.g. for stable sorting; local conversions LocalInternedString is used in those places.

r? @eddyb

nnethercote added 7 commits Oct 18, 2019
This avoids the needs for various conversions, and makes the code
slightly faster, because `Symbol` comparisons and hashing is faster.
It's a full conversion, except in `DefKey::compute_stable_hash()` where
a `Symbol` now is converted to an `InternedString` before being hashed.
This was necessary to avoid test failures.
This requires changing the `PartialOrd`/`Ord` implementations to look at
the chars rather than the symbol index.
This is a straightforward replacement except for two places where we
have to convert to `LocalInternedString` to get a stable sort.
By using `LocalInternedString` instead for the few remaining uses.
@eddyb
eddyb approved these changes Oct 21, 2019
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 21, 2019

@bors try (for perf)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 21, 2019

⌛️ Trying commit 08e2f05 with merge c7f7350...

bors added a commit that referenced this pull request Oct 21, 2019
Remove `InternedString`

This PR removes `InternedString` by converting all occurrences to `Symbol`. There are a handful of places that need to use the symbol chars instead of the symbol index, e.g. for stable sorting; local conversions `LocalInternedString` is used in those places.

r? @eddyb
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 21, 2019

Oh, btw, I think LocalInternedString would make more sense if it was called SymbolContents.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 21, 2019

Oh, btw, I think LocalInternedString would make more sense if it was called SymbolContents.

I agree! I was planning to rename in a follow-up. I had thought of SymbolChars but SymbolContents is also good.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 21, 2019

BTW, you can now do !bors try !rust-timer queue (but with @ instead of !) to queue a try run and a perf run in one command.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 21, 2019

☀️ Try build successful - checks-azure
Build commit: c7f7350 (c7f73500c3bcd5b6268b7c1ed4b14d78ee82ce59)

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 21, 2019

@rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 21, 2019

Awaiting bors try build completion

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 21, 2019

Queue needs to be run before the try build completes; @rust-timer build c7f7350

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 21, 2019

Queued c7f7350 with parent 1ba7b4e, future comparison URL.

@eddyb

This comment was marked as outdated.

Copy link
Member

eddyb commented Oct 21, 2019

Oops, I don't think that works?
@rust-timer build c7f7350

@rust-timer

This comment was marked as outdated.

Copy link

rust-timer commented Oct 21, 2019

Queued c7f7350 with parent 1ba7b4e, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 21, 2019

Finished benchmarking try commit c7f7350, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 22, 2019

Instruction counts look good, lots of small reductions of up to 1%. That's not surprising; hashing and ordering the symbol index is clearly cheaper than the symbol chars (which also involve a TLS lookup).

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Oct 22, 2019

Just to be sure: Hash and Eq are still based on the same data for each type? If the two disagree it can lead to subtle, hard to detect bugs.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 22, 2019

Just to be sure: Hash and Eq are still based on the same data for each type? If the two disagree it can lead to subtle, hard to detect bugs.

For Symbol both Hash and Eq are derived, and therefore both work with the index. And LocalInternedString doesn't implement Hash. So everything should be fine.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Oct 22, 2019

Great! Thanks for confirming, @nnethercote.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 22, 2019

@eddyb gave me r+ on Discord.

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2019

📌 Commit 08e2f05 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

Centril added a commit to Centril/rust that referenced this pull request Oct 23, 2019
…rly, r=eddyb

Remove `InternedString`

This PR removes `InternedString` by converting all occurrences to `Symbol`. There are a handful of places that need to use the symbol chars instead of the symbol index, e.g. for stable sorting; local conversions `LocalInternedString` is used in those places.

r? @eddyb
bors added a commit that referenced this pull request Oct 23, 2019
Rollup of 9 pull requests

Successful merges:

 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65666 (Deprecated proc_macro doesn't trigger warning on build library)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

 - #65625 (Turn crate store into a resolver output)

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Oct 23, 2019
…rly, r=eddyb

Remove `InternedString`

This PR removes `InternedString` by converting all occurrences to `Symbol`. There are a handful of places that need to use the symbol chars instead of the symbol index, e.g. for stable sorting; local conversions `LocalInternedString` is used in those places.

r? @eddyb
bors added a commit that referenced this pull request Oct 23, 2019
Rollup of 12 pull requests

Successful merges:

 - #64178 (More Clippy fixes for alloc, core and std)
 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro)
 - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Oct 24, 2019
Rollup of 12 pull requests

Successful merges:

 - #64178 (More Clippy fixes for alloc, core and std)
 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro)
 - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

r? @ghost
@bors bors merged commit 08e2f05 into rust-lang:master Oct 24, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191021.26 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
flip1995 added a commit to Mark-Simulacrum/rust-clippy that referenced this pull request Oct 24, 2019
@nnethercote nnethercote deleted the nnethercote:rm-InternedString-properly branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.