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

Misc changes from my parallel rustc branch #49030

Merged
merged 9 commits into from
Mar 23, 2018
Merged

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 14, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2018
@michaelwoerister
Copy link
Member

"Make Span and Symbol implement Send and Sync" (04e2088) is the only change that worries me a little. Since #48811 landed, it should be correct to share these between threads (with and without cfg(parallel_queries)). You would just get a runtime error when you try to look something up in an interner. So why do things differently for cfg(parallel_queries)?

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 16, 2018

@michaelwoerister It is always memory-safe to share them between threads. However it only actually works properly if you use them with the same interner and point the scoped thread local to the right interner on other threads. With cfg(not(parallel_queries)), we only use the interner on one thread, so using a Span or Symbol on other threads would result in a panic or a reference to the wrong thing, removing the Send and Sync impls gives us a compile time error instead.

@michaelwoerister
Copy link
Member

Can you add a comment that explains why the #[cfg] is there, i.e. that it's just an additional convenience in the single-threaded case?

Ideally we'll have lifetimes on everything that's interned. That would make us not depend on runtime errors.

@Zoxc Zoxc force-pushed the misc branch 2 times, most recently from 85d6ee8 to d1fae9b Compare March 17, 2018 07:42
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 17, 2018

@michaelwoerister I added comment about that.

I also removed a change which caused a panic on Travis. I couldn't reproduce it locally so I just removed it for now.

@michaelwoerister
Copy link
Member

Thanks, @Zoxc, I'll re-review asap.

@michaelwoerister
Copy link
Member

Looks good to me, thanks, @Zoxc!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2018

📌 Commit e09c2ff has been approved by michaelwoerister

@bors 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 Mar 19, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2018
bors added a commit that referenced this pull request Mar 23, 2018
@alexcrichton alexcrichton merged commit e09c2ff into rust-lang:master Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants