Skip to content

Send sync implementation on iterators #27615

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 12, 2015
Merged

Send sync implementation on iterators #27615

merged 2 commits into from
Aug 12, 2015

Conversation

GuillaumeGomez
Copy link
Member

Part of #22709.
cc @Veedrac

r? @bluss

@bluss
Copy link
Member

bluss commented Aug 9, 2015

We'd only need Send + Sync on table::RawBuckets, and the rest should follow by itself. Is that feasible @gankro? (RawTable uses Unique and should be fine already).

I had another question for you, do you know why RawBucket is using PhantomData<(K, V)>? I don't think it owns a K, but I don't have it all clear like you.

@Gankra
Copy link
Contributor

Gankra commented Aug 9, 2015

@bluss it ptr::read's out K,V pairs. Not sure if that necessitates it... this code has seen a lot of changes in the Phantom story.

RawBuckets should... basically be fine to impl for? Doesn't really matter since it's internal.

@GuillaumeGomez
Copy link
Member Author

So... What should I add/remove/update ?

@bluss
Copy link
Member

bluss commented Aug 9, 2015

@gankro It matters if all the things using RawBuckets would automatically inherit Send + Sync

@Gankra
Copy link
Contributor

Gankra commented Aug 9, 2015

Nothing in hashmap uses shared state/internal mutability so it's all trivially thread-safe, as far as I know.

@bluss
Copy link
Member

bluss commented Aug 9, 2015

@GuillaumeGomez If you impl Send + Sync for RawBuckets instead, all the rest should be inherited automatically.

@bluss
Copy link
Member

bluss commented Aug 9, 2015

Can you add the iterators to sync-send-iterators-in-libcollections.rs? (Or are they somewhere else?)

@GuillaumeGomez
Copy link
Member Author

@bluss: sure !

@GuillaumeGomez
Copy link
Member Author

I added the test.

@bluss
Copy link
Member

bluss commented Aug 9, 2015

Thanks! I'd prefer you squash the commits together to clean it up.

r=me with squash

huonw
huonw reviewed Aug 10, 2015
View reviewed changes
src/libstd/collections/hash/table.rs
@@ -741,6 +741,9 @@ struct RawBuckets<'a, K, V> {
marker: marker::PhantomData<&'a ()>,
}

unsafe impl<'a, K: Send, V: Send> Send for RawBuckets<'a, K, V> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it's not obvious to me that this is the right thing. Maybe this should be K: Sync, V: Sync? Basically, I'm asking what reference/value semantics does RawBuckets have with K and V?

@bluss bluss assigned huonw and unassigned bluss Aug 10, 2015
@bluss
Copy link
Member

bluss commented Aug 10, 2015

Giving this issue to huon, who is much more experienced in the details of correct Sync and Send.

@bluss
Copy link
Member

bluss commented Aug 10, 2015

So, my plan make this easier by impl on RawBuckets was misguided, and this is because the different iterators do actually need different bounds. The Send/Sync impl for RawBuckets is not correct -- its behavior depends on use.

For reference, see this comment for the different cases. The guidelines should be directly applicable since the HashMap does not do any funny business with their contents during iteration.

We only need to impl Send / Sync for the basic building blocks. In this case this is table::{Iter, IterMut, IntoIter, Drain} inside src/libstd/collections/hash/table.rs. The HashMap and HashSet iterators will inherit Send / Sync from these.

  • impl<'a, K: Sync, V: Sync> Sync for Iter<'a, K, V>
  • impl<'a, K: Sync, V: Sync> Send for Iter<'a, K, V>
  • impl<'a, K: Sync, V: Sync> Sync for IterMut<'a, K, V>
  • impl<'a, K: Sync, V: Send> Send for IterMut<'a, K, V> // **Note**
  • impl<K: Sync, V: Sync> Sync for IntoIter<K, V>
  • impl<K: Send, V: Send> Send for IntoIter<K, V>
  • impl<'a, K: Sync, V: Sync> Sync for Drain<'a, K, V>
  • impl<'a, K: Send, V: Send> Send for Drain<'a, K, V>

huonw
huonw reviewed Aug 10, 2015
View reviewed changes
src/libstd/collections/hash/set.rs
@@ -764,18 +764,27 @@ pub struct Iter<'a, K: 'a> {
iter: Keys<'a, K, ()>
}

unsafe impl<'a, K: Sync> Sync for Iter<'a, K> {}
unsafe impl<'a, K: Send> Send for Iter<'a, K> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be K: Sync, since Iter yields/behaves like &K.

@GuillaumeGomez GuillaumeGomez force-pushed the send_sync branch 2 times, most recently from 97ee72b to 2709683 Compare August 10, 2015 20:14
alexcrichton
alexcrichton reviewed Aug 10, 2015
View reviewed changes
src/libstd/collections/hash/set.rs
@@ -764,18 +764,27 @@ pub struct Iter<'a, K: 'a> {
iter: Keys<'a, K, ()>
}

unsafe impl<'a, K: Sync> Sync for Iter<'a, K> {}
unsafe impl<'a, K: Sync> Send for Iter<'a, K> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be automatically inferred by applying the traits to the underlying Keys iterator?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er actually, all of the modifications in this file should not be necessary as they're built on top of the map iterators

@huonw
Copy link
Member

huonw commented Aug 11, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 11, 2015

📌 Commit f2f4a5c has been approved by huonw

bors added a commit that referenced this pull request Aug 12, 2015
@bors
Auto merge of #27615 - GuillaumeGomez:send_sync, r=huonw
542d56e 
Part of #22709.
cc @Veedrac

r? @bluss
@bors
Copy link
Collaborator

bors commented Aug 12, 2015

⌛ Testing commit f2f4a5c with merge 542d56e...

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Aug 12, 2015
@alexcrichton
rollup merge of rust-lang#27615: GuillaumeGomez/send_sync
a0d5181 
Part of rust-lang#22709.
cc @Veedrac

r? @bluss
@bors
Copy link
Collaborator

bors commented Aug 12, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit f2f4a5c into rust-lang:master Aug 12, 2015
@bors bors mentioned this pull request Aug 12, 2015
Merged
@GuillaumeGomez GuillaumeGomez deleted the send_sync branch August 12, 2015 10:11
@huonw huonw mentioned this pull request Aug 12, 2015
Closed
30 tasks
@sparecycles sparecycles mentioned this pull request Dec 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@huonw huonw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @bluss @Gankra @huonw @bors @alexcrichton