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

Mechanical translation towards making GlobalCtxt implement Sync #46958

Closed
wants to merge 1 commit into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Dec 23, 2017

This PR is split out from #45912.

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

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

@bors
Copy link
Contributor

bors commented Dec 23, 2017

☔ The latest upstream changes (presumably #46842) 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
@alexcrichton
Copy link
Member

ping @michaelwoerister, did you want to take a look at this?

@michaelwoerister
Copy link
Member

Nominating for discussion in the @rust-lang/compiler team meeting. This is a huge change that I can't approve by myself. Also, I think we should have a broader, more open discussion about multi-threading support in the compiler. I feel that such a thing would need an RFC.

@eddyb
Copy link
Member

eddyb commented Jan 5, 2018

I've insistented on numerous occasions on IRC that rustc is currently optimized for straight-line and single-threaded performance and we'd have to redesign core parts of it for parallelism.

It is plausible that you get some speedup even with locks everywhere but it's not ideal IMO.

@nikomatsakis
Copy link
Contributor

I do feel like parallelizing rustc seems like a good goal but I would like to see some more design discussion.

That said, I don't know that I have an objection to this PR in particular, which seems like it's probably necessary groundwork? If I understand what's happening here, we're basically just redirecting names on the basis of a configuration option? (i.e., we're still going to be using Rc and RefCell).

That said, I think I might prefer to use names Rc and RefCell, but just import them from rustc_data_structures::sync. Then the diff would be quite small (we just modify the use lines, and that's it), but we achieve the same ends. We can always do The Big Edit at some later point.

Thoughts?

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 6, 2018

That said, I don't know that I have an objection to this PR in particular, which seems like it's probably necessary groundwork? If I understand what's happening here, we're basically just redirecting names on the basis of a configuration option? (i.e., we're still going to be using Rc and RefCell).

That is indeed what is happening.

That said, I think I might prefer to use names Rc and RefCell, but just import them from rustc_data_structures::sync. Then the diff would be quite small (we just modify the use lines, and that's it), but we achieve the same ends. We can always do The Big Edit at some later point.

There are a couple of problems with that.

  • There are still uses of Rc, Cell and RefCell which do not get redirected to multi-threaded variants so its easy to mix them up. This is particularly problematic if some new use of Rc or RefCell gets added which refer to the multi-threaded variant, but actually don't require the use of a multi-threaded variant. This means that locks can silently get added. On the other hand, if we use distinct names for the single-threaded and multi-threaded variants, this isn't likely to happen.
  • LockCell does not get redirected to Cell, but is a thin wrapper around Cell so that it has the same API as the multi-threaded variant.
  • Lock doesn't have a corresponded single threaded variant. RwLock corresponds to RefCell and allows multiple readers at once, while Lock only allows one access at a time. This PR only uses RwLock where multiple readers are required and Lock otherwise.

@michaelwoerister michaelwoerister added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2018
@nikomatsakis
Copy link
Contributor

We discussed this in the @rust-lang/compiler meeting. Consensus was that @Zoxc would open a thread on internals and we would try to ensure we have some level of agreement on the overall plan before we go forward. Closing PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants