-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
More thread-safety changes #49882
More thread-safety changes #49882
Conversation
☔ The latest upstream changes (presumably #49558) made this pull request unmergeable. Please resolve the merge conflicts. |
Could the arena case not be handled by wrapping arenas in a mutex instead of hard-coding the mutex into the arena types? This way one could decide on a case-by-case basis if an arena needs to be synchronized or not. |
No, arenas and mutexes do not compose. We could have a thread-safe and thread-unsafe variants of the arenas however. That would probably reduce the amount of changed lines too. |
Can you elaborate on that? Do you mean that one can't use a thread-unsafe arena where a thread-safe arena is expect? |
If you use a |
☔ The latest upstream changes (presumably #49396) made this pull request unmergeable. Please resolve the merge conflicts. |
4eb94b3
to
04c213c
Compare
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I added thread-safe variant of the arenas instead of modifying the existing types. |
Thanks, I think that's an improvement. |
@@ -1554,7 +1554,7 @@ impl<'gcx: 'tcx, 'tcx> GlobalCtxt<'gcx> { | |||
/// Call the closure with a local `TyCtxt` using the given arena. | |||
pub fn enter_local<F, R>( | |||
&self, | |||
arena: &'tcx DroplessArena, | |||
arena: &'tcx SyncDroplessArena, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the local arenas in theory be non-synchronized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the arenas could be thread-local if we find a way to deal with lifting of types.
📌 Commit fe63637 has been approved by |
More thread-safety changes r? @michaelwoerister
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister