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

More thread-safety changes #49882

Merged
merged 6 commits into from
Apr 17, 2018
Merged

More thread-safety changes #49882

merged 6 commits into from
Apr 17, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 11, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2018
@bors
Copy link
Contributor

bors commented Apr 12, 2018

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

@michaelwoerister
Copy link
Member

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.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 13, 2018

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.

@michaelwoerister
Copy link
Member

Can you elaborate on that? Do you mean that one can't use a thread-unsafe arena where a thread-safe arena is expect?

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 13, 2018

If you use a Mutex<Arena> the lifetimes of the allocated objects will be restricted to the mutex guard.

@bors
Copy link
Contributor

bors commented Apr 14, 2018

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

@Zoxc Zoxc force-pushed the sync-misc2 branch 3 times, most recently from 4eb94b3 to 04c213c Compare April 15, 2018 14:42
@rust-highfive
Copy link
Collaborator

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.
[00:00:46] configure: rust.quiet-tests     := True
---
[00:05:31] error[E0596]: cannot borrow immutable borrowed content as mutable
[00:05:31]    --> libarena/lib.rs:447:9
[00:05:31]     |
[00:05:31] 447 |         self.lock.lock().clear();
[00:05:31]     |         ^^^^^^^^^^^^^^^^ cannot borrow as mutable
[00:05:31]
[00:05:31] error: aborting due to previous error
[00:05:31]
[00:05:31] For more information about this error, try `rustc --explain E0596`.
[00:05:31] error: Could not compile `arena`.
[00:05:31]
[00:05:31] Caused by:
[00:05:31]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name arena libarena/lib.rs --color always --error-format json --crate-type dylib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 -C metadata=dd80e21b25e89693 -C extra-filename=-dd80e21b25e89693 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release/deps --extern rustc_data_structures=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_data_structures-93cb1ddd29ab61a4.so` (exit code: 101)

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 @TimNN. (Feature Requests)

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 15, 2018

I added thread-safe variant of the arenas instead of modifying the existing types.

@michaelwoerister
Copy link
Member

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,
Copy link
Member

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?

Copy link
Contributor Author

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.

@michaelwoerister
Copy link
Member

Looks good to me. Thanks, @Zoxc!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2018

📌 Commit fe63637 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 Apr 16, 2018
@bors
Copy link
Contributor

bors commented Apr 17, 2018

⌛ Testing commit fe63637 with merge 6b12d36...

bors added a commit that referenced this pull request Apr 17, 2018
@bors
Copy link
Contributor

bors commented Apr 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 6b12d36 to master...

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.

4 participants