Skip to content

Store a Symbol in InternedString #46972

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Store a Symbol in InternedString #46972

wants to merge 2 commits into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Dec 23, 2017

No description provided.

@rust-highfive
Copy link
Contributor

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@Zoxc Zoxc force-pushed the internedstring-no-deref branch from c758e13 to 100c61b Compare December 24, 2017 04:22
@eddyb
Copy link
Member

eddyb commented Dec 24, 2017 •
edited
Loading

r? @jseyfried (I don't think we want these changes but we probably need a wider discussion)

EDIT: InternedString, before this PR, might be unsound, wrt placing it in a thread_local.
However, that wouldn't be the case if the interner is owned outside of the threads using it.

@rust-highfive rust-highfive assigned jseyfried and unassigned pnkfelix Dec 24, 2017
@bors
Copy link
Collaborator

bors commented Dec 25, 2017

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

@Zoxc Zoxc force-pushed the internedstring-no-deref branch from 100c61b to 4cda813 Compare December 25, 2017 21:16
@Zoxc Zoxc changed the title WIP: Store a Symbol in InternedString Store a Symbol in InternedString Dec 25, 2017
@bors
Copy link
Collaborator

bors commented Dec 27, 2017

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2017
@Zoxc Zoxc force-pushed the internedstring-no-deref branch from 4cda813 to c3ab779 Compare December 28, 2017 03:03
@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 28, 2017

@bors try

bors added a commit that referenced this pull request Dec 28, 2017
@bors
Auto merge of #46972 - Zoxc:internedstring-no-deref, r=<try>
8c52267 
Store a Symbol in InternedString
@bors
Copy link
Collaborator

bors commented Dec 28, 2017

⌛ Trying commit c3ab779 with merge 8c52267...

@bors
Copy link
Collaborator

bors commented Dec 28, 2017

☀️ Test successful - status-travis
State: approved= try=True

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 28, 2017

@Mark-Simulacrum I'd like a perf run on this

@Mark-Simulacrum
Copy link
Member

Perf run queued.

@alexcrichton
Copy link
Member

ping @Mark-Simulacrum, do we have perf results now perchance?

@pnkfelix
Copy link
Member

pnkfelix commented Jan 4, 2018

(Also, Note that @jseyfried has not yet weighed in on whether they want these changes.)

@Mark-Simulacrum
Copy link
Member

http://perf.rust-lang.org/compare.html?start=6c06bfaebad19abc73506e730e97b03291ecf187&end=8c522672d3f1dab4c6af8a0bd3cf8c731c5f9c8e&stat=instructions%3Au

@carols10cents
Copy link
Member

carols10cents commented Jan 9, 2018 •
edited
Loading

review ping @jseyfried ! pinging you on IRC too! or not because i don't see you :)

@kennytm kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 17, 2018
@kennytm
Copy link
Member

kennytm commented Jan 17, 2018

Review ping for you @jseyfried! Also cc @rust-lang/compiler.

@nikomatsakis
Copy link
Contributor

Can somebody summarize (a) what is being done here so I don't have to read the diffs and (b) why I should care?

@eddyb
Copy link
Member

eddyb commented Jan 17, 2018 •
edited
Loading

@nikomatsakis This is reverting an API change @jseyfried (I believe) did.
Right now we deref InternedString to str (sound because InternedString is not Send/Sync).
@Zoxc moves us backwards to an with_str style API with this PR (which makes InternedString largely redundant with Symbol - IMO InternedString should be e.g. SymbolContents anyway).

@petrochenkov
Copy link
Contributor

What's the point of InternedString as a separate type if it's equivalent to Symbol now?

So far I didn't look carefully at @Zoxc's multithreading-related PRs and mostly seen them as something quite unfortunate, but maybe necessary after all.
Could someone write a design summary about how Symbols are supposed to be organized in multithreaded rustc?
Is there going to be a global interner? Or many thread-local interners with string duplication? Or both?

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 17, 2018

What's the point of InternedString as a separate type if it's equivalent to Symbol now?

It has Ord and Hash implementations based on strings instead of an unstable integer like Symbol.

Is there going to be a global interner? Or many thread-local interners with string duplication? Or both?

My branch has a interner per compilation session which is behind a lock. There is no parallelism during parsing there though, so all symbols are identical to what happens on master.

@michaelwoerister
Copy link
Member

This seems to be slowing things down quite a bit (as per the perf run above). Is this a soundness fix? Is there an alternative implementation that allows us to keep using direct string references?

@eddyb
Copy link
Member

eddyb commented Jan 18, 2018

I believe we can just require that the interner outlives the threads that use it.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 18, 2018

@michaelwoerister I suspect the slowdown is due to the usages of to_string() here. Most of these are because we want to match the interned string on constant strings and there's some return inside so we don't want a closure. We should instead pre-intern these constant strings using a macro. This avoids the string allocation and the string comparisons.

@eddyb I think that spawning a thread for the interner should work. Not terribly elegant though.

@eddyb
Copy link
Member

eddyb commented Jan 18, 2018

@Zoxc I mean you already have the threads, you just have to own the interner outside of them.

@carols10cents carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2018
@nikomatsakis
Copy link
Contributor

We should instead pre-intern these constant strings using a macro. This avoids the string allocation and the string comparisons.

It seems pretty important to resolve this, no?

@shepmaster
Copy link
Member

Thanks for the PR, @Zoxc ! Since we haven't heard from you in a few weeks, I'm going to close this for now. Please feel free to reopen once you address the merge conflicts and the last round of feedback!

@shepmaster shepmaster closed this Feb 3, 2018
@shepmaster shepmaster added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2018
@petrochenkov petrochenkov mentioned this pull request Mar 22, 2018
Closed
This was referenced Apr 9, 2018
Merged
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Apr 27, 2018
@kennytm
Rollup merge of rust-lang#49894 - Zoxc:sync-internedstring, r=michael…
44b5359 
…woerister

Rename InternedString to LocalInternedString and introduce a new thread-safe InternedString

This is an allocation-free alternative to rust-lang#46972.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 10, 2018
@alexcrichton
Rollup merge of rust-lang#49823 - Zoxc:term-str, r=alexcrichton
54018b1 
Remove usages of Term::as_str and mark it for removal

Returning references to rustc internal data structures is a bad idea since their lifetimes are unrelated to the lifetimes of proc_macro values.

See rust-lang#46972 and the `Taming thread-local storage` section of https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606

r? @alexcrichton
bors added a commit that referenced this pull request May 10, 2018
@bors
Auto merge of #49823 - Zoxc:term-str, r=alexcrichton
9e3caa2 
Remove usages of Term::as_str and mark it for removal

Returning references to rustc internal data structures is a bad idea since their lifetimes are unrelated to the lifetimes of proc_macro values.

See #46972 and the `Taming thread-local storage` section of https://internals.rust-lang.org/t/parallelizing-rustc-using-rayon/6606

r? @alexcrichton
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@jseyfried jseyfried

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.