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

Add rayon parallel sort example #233

Merged
merged 1 commit into from Aug 15, 2017

Conversation

Projects
None yet
2 participants
@lilianmoraru
Copy link
Contributor

lilianmoraru commented Jul 9, 2017

I initially went with an example like this:

extern crate rand;
extern crate rayon;

use rand::Rng;
use rayon::prelude::*;

fn main() {
    let mut vec = vec![0i64; 1_000_000];
    
    // Generating random numbers to be sorted later
    vec.par_iter_mut().for_each(|p| *p = rand::weak_rng().gen::<i64>());
    
    // Using Rayon's Parallel QuickSort to sort the vector
    vec.par_sort_unstable();
}

but that seemed overly simplistic(I think you are more likely to sort strings than just integers).

Had no idea what to write in the description, under the code...

@budziq
Copy link
Collaborator

budziq left a comment

@lilianmoraru well done 👍 just some minor nits below

@@ -223,8 +256,11 @@ Lastly, collects calculation results via [`mpsc::channel`] with [`Receiver::recv
[`Receiver::recv`]: https://doc.rust-lang.org/std/sync/mpsc/struct.Receiver.html#method.recv
[`Rgb::from_channels`]: https://docs.rs/image/*/image/struct.Rgb.html#method.from_channels
[`Scope::spawn`]: https://docs.rs/crossbeam/0.*/crossbeam/struct.Scope.html#method.spawn
[`stable sorting`]: https://docs.rs/rayon/0.8/rayon/slice/trait.ParallelSliceMut.html#method.par_sort

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

please use wildcard "*" doc version in links to always point to latest docs

This comment has been minimized.

@lilianmoraru

lilianmoraru Jul 11, 2017

Author Contributor

I am worried that the API might change.
This particular function was added in the latest 0.8.2.
If you think that it is ok, I can switch it to the wildcard.

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

Oh these are expected to break eventually as most other examples, that is why all examples are built and tested nightly with wildcard dependencies in Cargo.toml to catch such buildbreaks. Granted it increases the maintenance burden but its unavoidable in a resource spanning 30+ crates.

This comment has been minimized.

@lilianmoraru

lilianmoraru Jul 11, 2017

Author Contributor

changed and pushed

@@ -30,6 +31,36 @@ The example uses the Rayon crate, which is a data parallelism library for Rust.
Rayon provides the `par_iter_mut()` method for any parallel iterable data type.

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

could you also cleanup this par_iter_mut() reference while you're at it?

  • Remove the () suffix
  • link it to docs

This comment has been minimized.

@lilianmoraru

lilianmoraru Jul 11, 2017

Author Contributor

fixed locally

}
```

The example uses the `rayon` crate, which is a data parallelism library for Rust.<br/>

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

I would remove the first sentence. We already have a trivial example above (which needs a major description rewrite anyway - as does rest of this section).

The more typical form of textual description would be to briefly describe what is actually achieved in the code.
"Example prepares an unsorted vector...".
I would also mention/describe the data preparation method as it is more interesting by itself than the basic example above 😄

This comment has been minimized.

@lilianmoraru

lilianmoraru Jul 11, 2017

Author Contributor

I moved the explanations inside the code, I think this is a better way to present the code - it's easier for the user to follow about which part of the code you are talking about(compared to the after the fact - style) - inspired from how Qt examples are structured(when you open them in QtCreator, not on the website).
I still left that part at the bottom for "side notes".

I write all this here because I will push the modifications and you might not like it.
I make my point, in case you agree.
If you don't, I will make the modifications how you think they should be presented(iterative feedback expected here :) ).

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

I would suggest to move the descriptions out of the code into a terse textual description preceding the code. Bare with me (I'll describe why in next comment)

compared to the after the fact - style

This is why we follow prior the fact style in other sections https://brson.github.io/rust-cookbook/net.html 😉 . But seriously the concurrency section has the most legacy cruft accumulated an will require a rewrite anyway.

```

The example uses the `rayon` crate, which is a data parallelism library for Rust.<br/>
We have [`multiple options`] to sort an Iterable data type, we chose here to use [`par_sort_unstable`]

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

I would use [`identifier`] link types only for identifiers. These links already point to interesting parts of docs to please just change the text to the pointed to identifier and modify the textual description to keep the sentences coherent.

This comment has been minimized.

@lilianmoraru

lilianmoraru Jul 11, 2017

Author Contributor

This is again inspired from the Qt docs(which I think are considerably better than the Rust docs).
I like when docs give more context and have optional links you can go to, to get more info about the topic - I don't think that it has to be strictly types only.
Here is one example from Qt: http://doc.qt.io/qt-5/qvector.html#details - pay attention that you have links to: reentrant(which explains the difference between thread-safe and reentrace, nothing about Qt types), container classes(which gives you more context about other types of containers), constant time(which gives you more context about the algorithmic complexity of different Qt data structures operations), default-constructed value(...), implicitly shared copies(which explains you COW in Qt).
Compared to Rust, this style of docs never left me feeling that I don't have all the info I need.

In summary: I think this is helpful to the user.

This comment has been minimized.

@budziq

budziq Jul 11, 2017

Collaborator

Actually I am quite aware of Qt docs as I've used it more than a few times and I can give a even better example of developer docs as Imho Apple developer docs are the gold standard on terms of quality and completeness (hmm somehow the Qt4 docs always seamed nicer than the Qt5 ones. Probably Nokia had more focus on quality then but I'm beside the point 😸).

But the real point being is that the cookbook is not an API/language nor crate doc (and your points are excellent for such types of resources ). Cookbook is an introductory (and in the future hopefully intermediate) resource assisting problem solving and crate discoverability (and lastly some good pointers on pythonic rustic code).

The typical use case for our reader is intended to:

  1. quickly search for solution to a given problem - we are limping here ATM
  2. see what are the actors / crates for typical idiomatic solution - with links to the real docs allowing reader to educate oneself
  3. copy the minimal snippet to their code base

In such light the possible educational value is assumed to come from the linked docs.

Please note that, this is the current working idea that we are following but we are open for suggestions. And we would love to hear yours and other developer's thoughts on this matter. But any change to the cookbook formula will have to be discussed first and then implemented consistently throughout the whole cookbook.

So to summarize. We plan on tweaking the cookbook formula in the future (both content and form) and would love for you to take part in the discussion.

@lilianmoraru

This comment has been minimized.

Copy link
Contributor Author

lilianmoraru commented Jul 11, 2017

Pushed the fixes with some of the points unaddressed(rayon = "*" & links), because I want to see if I manage to convince you otherwise.

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Jul 11, 2017

I want to see if I manage to convince you otherwise.

No need for convincing. I'm already a fan of tutorial teaching style resources. And one of the ideas is to eventually expand cookbook to "Rust By Example" style or even merge with it. But we are currently brainstorming for ideas

@lilianmoraru

This comment has been minimized.

Copy link
Contributor Author

lilianmoraru commented Jul 12, 2017

Sorry, I'm low on time right now.
I'll follow-up on this tommorow

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Jul 13, 2017

No rush :)

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Jul 29, 2017

Hi @lilianmoraru. I'm back from holidays so I should be to have a more meaningful discussion in regard to this example. Are you willing to continue your work on it?

@lilianmoraru

This comment has been minimized.

Copy link
Contributor Author

lilianmoraru commented Aug 15, 2017

I was also on vacation.
Let's continue with this.

A few things to note here:

  1. I rebased on the current master
  2. I moved the other explanations under concurrency, to be on top of the code(more consistent with the recent changes in the other examples).
  3. I removed the explanations from inside the code as requested, but the explanation was so disconnected from the code that I myself would not understand what it was talking about(why I've decided to put the explanations there in the first place)... So, I've marked the areas(again, another style borrowed from Qt, this time from Qt examples), to show the reader about which areas of the code the explanation is talking about.
    I still think the explanations should be right beside the code, so the user is connected better to what the explanation talks about - but I won't insist, as long as this helps to close this PR(be it merged or not).

I was planning to remove the multiple options and stable sorting links(which I think are important to the reader, ex: comes to see how to sort, but wants a stable sorting algorithm, lucky him, he had a link to the other sorting algorithms and didn't think there are no options for him), but I saw the Julia Set link and left them in(again, because I think that we should leave more info/options to the reader).

@budziq budziq merged commit 2ddd4e4 into rust-lang-nursery:master Aug 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Aug 15, 2017

Merged 😃 Thanks @lilianmoraru!

Thanks for the cleanup It was on my todo list for way to long :)

I'm not sure about the numbered reference-pseudo-links as we lack proper tooling to turn them into a real hyperlink or a split code/tutorial page like in qt examples case, making it look a little awkward and nonconformant with other examples style. But we are soliciting ideas for design and form overhaul so I'm willing to bend the rules here to get some input from users :)

If you are willing to add some input to the discussion, you are very welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.