Implement Rayon traits (optional) #45

cuviper · 2017-11-20T19:40:54Z

IntoParallelIterator by value, ref, and ref mut
- Implements both ParallelIterator and IndexedParallelIterator
FromParallelIterator
ParallelExtend by value and ref copy

bluss · 2017-11-20T20:38:18Z

src/rayon.rs

+            vec
+        })
+        .collect()
+}


Loll wut!! LinkedList is used in a real-life PR?!! 🥇 🐫 🎈

(Ahem I'll be back to my regular @bluss in a bit.)

You know, it's funny, for all the strategies we benchmarked for Rayon collect, I don't think we actually tried a Vec<Vec<T>> intermediate. I just assumed that a list was best for this purpose because of its O(1) append. I'm going to have to do some testing now...

I've kept the LinkedList, but now collect it with map-reduce -- see rayon-rs/rayon#479.

bluss

Hi, I'll review this sooner or later. Not Either-PR late. Just not tonight. Thanks for the solid PR.

bluss · 2017-11-20T20:39:38Z

src/rayon.rs

+
+        // NB: This allows indexed collection, e.g. directly into a `Vec`, but the
+        // underlying iterator must really be indexed.  We should remove this if we
+        // start having tombstones that must be filtered out.


Great comment.

@bluss

479: ParallelExtend with map-reduce r=cuviper a=cuviper We were using a `LinkedList<Vec<_>>` intermediary for general `ParallelExtend` implementations, gathered using a `fold` to a vector and `collect` the list -- effectively a `fold`-`fold`-`reduce`. Now we explicitly `fold`-`map`-`reduce` it for better performance. Thanks to @bluss for [ridiculing](indexmap-rs/indexmap#45 (comment)) the use of `LinkedList`, which got me to look at this again. At first I tried `Vec<Vec<_>>` instead, which did perform better, but I had to do this using `fold`-`map`-`reduce` to avoid infinite `collect` recursion. Then I tried that same tweak on the original code, finding it slightly better yet. So I stand by the use of `LinkedList`. 😄 ``` test map_collect::i_mod_10_to_i::with_linked_list_collect_vec_sized ... bench: 5,112,790 ns/iter (+/- 78,988) test map_collect::i_mod_10_to_i::with_linked_list_map_reduce_vec_sized ... bench: 4,954,336 ns/iter (+/- 103,285) test map_collect::i_mod_10_to_i::with_vec_vec_sized ... bench: 4,965,241 ns/iter (+/- 80,797) test map_collect::i_to_i::with_linked_list_collect_vec_sized ... bench: 9,837,088 ns/iter (+/- 283,665) test map_collect::i_to_i::with_linked_list_map_reduce_vec_sized ... bench: 9,687,170 ns/iter (+/- 328,236) test map_collect::i_to_i::with_vec_vec_sized ... bench: 9,690,253 ns/iter (+/- 242,379) test vec_collect::vec_i::with_linked_list_collect_vec_sized ... bench: 12,639,762 ns/iter (+/- 260,212) test vec_collect::vec_i::with_linked_list_map_reduce_vec_sized ... bench: 9,805,660 ns/iter (+/- 170,434) test vec_collect::vec_i::with_vec_vec_sized ... bench: 9,904,740 ns/iter (+/- 226,382) test vec_collect::vec_i_filtered::with_linked_list_collect_vec_sized ... bench: 12,486,357 ns/iter (+/- 239,281) test vec_collect::vec_i_filtered::with_linked_list_map_reduce_vec_sized ... bench: 9,794,605 ns/iter (+/- 306,361) test vec_collect::vec_i_filtered::with_vec_vec_sized ... bench: 9,986,733 ns/iter (+/- 1,142,244) ```

bluss · 2017-11-22T08:28:30Z

As awesome as this is, it does interfere with easy 1.0'ing of ordermap.

cuviper · 2017-11-22T18:16:19Z

Would you want to wait for rayon-1.0? Or would you hesitate even then?

Another idea is to add as_slice(), as_mut_slice(), and into_vec(), and put a public API on the Bucket, so people could themselves write something like:

omap.as_slice().into_par_iter()
    .map(|b| (b.key(), b.value())))
    .for_each(|(k, v)| ...);

That's not as ergonomic, but at least it would be possible to use Rayon with ordermap efficiently, and maybe enable other cool uses too!
(tombstones would make this uglier though.)

bluss · 2017-11-22T18:22:16Z

With rayon-1.0 I would not hesitate, but you're right, one could be worried about the version entanglement even then.

For the record, we've decided that OrderMap will stay as it is with its current properties, indexing, exact size = no tombstones (and probably the same name too, if we don't find a better one). This data structure is useful as it is, so to implement a similar one with tombstones, it will be a new thing. We could end up with both.

bluss · 2017-11-22T18:26:35Z

W.r.t order I'm very fond of a deterministic order (hash function independent).

I had a different thought -- would it be possible to implement shared and mutable views for OrderMap. So that it would natively support splitting (by index) into two parts or even slicing. Both the shared and mutable view would mean shared/mutable access to the elements only, not inserting or removing elements. I think that means that the mutable views can all have the same shared immutable reference to the index/hash vector.

The idea includes allowing lookup by key and indexing in a view.

In a mutable view, the implementation peeking into another view's part of the entries vector would be verboten.

cuviper · 2017-11-22T19:24:28Z

I think views are a neat idea! It wouldn't directly enable rayon, but it could let there be something like ordermap-parallel, like you did with ndarray and ndarray-parallel.

vitiral · 2018-01-03T23:18:10Z

Just a thought: IMO feature flags should be able to be independent of the crate stability. I.e. you can advertise that "rayon feature flag is supported but is beta".

bluss · 2018-01-03T23:36:33Z

Yes, maybe. I think the feature flag must have unstable in its name for it to work. It's different viewpoints among users in the role of docs vs formal API in defining the stable interface.

cuviper · 2018-02-16T09:23:12Z

I've now rebased, updated to rayon 1.0, and added the basic set implementations.

bluss · 2018-02-19T21:47:53Z

That's cool! I'm fine with merging this, but I haven't looked through it in fine detail. rayon 1.0 is quite the thing, and maybe we can dare to go 1.0 with it as dependency. (Congratulations!)

cuviper · 2018-02-19T23:16:32Z

I was also thinking of par_eq, par_is_disjoint, par_is_subset, and par_is_superset, as these are all operations that have to (potentially) iterate the whole set. This might be too much par_ though. 😜

The only thing left would be drain, which we haven't really addressed at all in rayon. I'm thinking about adding a general ParDrain trait, but I'm not sure.

Anyway, I should at least write some documentation and tests here before we merge. 😅

One specific API question for you is whether it's good to have these Par* types re-exported in the main map and set modules, next to their serial counterparts. It might be better in separate modules, perhaps even public from the indexmap::rayon::map/set where they're currently implemented, and remove Par from their names. (like Rayon does for std iterator types)

bluss · 2018-02-21T22:33:57Z

This PR is clearly amazing. I think this is a great feature set for a hashmap library and in safe Rust no less (thanks to the giants of std::vec::Vec and rayon).

I think that's a lot of functions. I'd probably drop the comparisons and par_sorted_by (sorted_by would probably not have existed if sort_by was realized first? It is a tad more efficient, than sort_by though..). I'm just wondering if we can get most of the value with less API.

I don't know about the module and name prefix question. Mirroring std fits well in the rayon crate. Here I don't mind either way.

ConnorGray · 2018-10-29T00:02:58Z

What's the status of this pull request? I'm a fan of IndexMap, but right now I'm in a situation where I'd really like to be able to use rayon to get a performance boost, and unfortunately can't.

cuviper · 2018-10-29T00:23:34Z

@ConnorGray -- It's kind of in that "Does anyone care?" limbo, so your direct request will help motivate me to revisit this. 🙂

I need to rebase it, at least, and then make some decision about those API questions. If you have any input, feel free to share your thoughts (but no worries if you're indifferent to the specifics).

ConnorGray · 2018-10-29T00:33:58Z

@cuviper Wow the quick response is much appreciated :)

I don't have a much input -- I'm just hoping to take advantage of rayon's accessible parallelism (where not having to worry about the details is a selling point) with the determinism I get from using an IndexMap everywhere internally.

I imagine there might be others in the same boat, who like IndexMap for it's determinism, but would also like to operate on the values in parallel.

bluss · 2018-11-21T20:51:36Z

I think this is good. Rayon is rock solid 😉 and doesn't feel like a risk. The code is simple.

Only review comment that is important to fix, is that the new methods (par_keys etc) need to be listed last in the docs, not first in the docs. I'd enable the rayon feature in docs.rs generation and make sure items are labelled in docs to require that feature (I can do that).

garro95 · 2019-04-29T13:33:08Z

What's the state of this? I would like to use this feature

cuviper · 2019-08-20T00:31:04Z

I think this is ready for reconsideration.

bluss

This is great, ready to merge! When the trivial conflict in .travis.yml is fixed

cuviper · 2019-08-20T17:54:39Z

OK, that conflict was just in the compat downgrades, which #97 removed.

Rebased without that, and CI passed -- r=bluss

bluss · 2019-08-20T18:03:54Z

Great. I can do the release of 1.1.0

cuviper mentioned this pull request Nov 20, 2017

Rayon integration #25

Closed

bluss reviewed Nov 20, 2017

View reviewed changes

bluss self-assigned this Nov 20, 2017

cuviper mentioned this pull request Nov 21, 2017

ParallelExtend with map-reduce rayon-rs/rayon#479

Merged

cuviper mentioned this pull request Nov 30, 2017

WIP: Add views of OrderMap #47

Closed

cuviper force-pushed the rayon branch from d7f2ca6 to 2eb2ff4 Compare February 16, 2018 09:16

cuviper force-pushed the rayon branch from 2eb2ff4 to 1ccb863 Compare February 16, 2018 09:33

cuviper force-pushed the rayon branch from 377cac4 to d516bdf Compare November 3, 2018 01:00

TerrariumCode mentioned this pull request Jun 10, 2019

Improve codegen performance rusoto/rusoto#1413

Closed

cuviper force-pushed the rayon branch from d516bdf to a02eccc Compare August 19, 2019 21:54

cuviper mentioned this pull request Aug 19, 2019

1.0.3 release? #96

Closed

cuviper force-pushed the rayon branch from a6d19e6 to 705d3ba Compare August 19, 2019 23:43

bluss approved these changes Aug 20, 2019

View reviewed changes

cuviper added 13 commits August 20, 2019 10:42

Implement Rayon traits (optional)

c5462e5

Add par_keys, par_values, par_values_mut

f1fd8a6

Add parallel set operators

777e908

Add parallel sorting

de6fe51

Add parallel comparisons

475fd81

Include rayon in docs.rs

2cba73d

Let rayon methods be documented last

1b4bcc4

Export parallel iterators under map::rayon and set::rayon modules

8a6bac1

Document parallel iterator types

4eb7d99

Document parallel methods

734f350

Add parallel tests based on the serial tests

97a34f1

impl Debug + Clone for parallel structs

46e3368

Note impls that requires rayon

85abcb1

cuviper force-pushed the rayon branch from 9f1d1c4 to 85abcb1 Compare August 20, 2019 17:43

cuviper merged commit c9fb28f into indexmap-rs:master Aug 20, 2019

cuviper deleted the rayon branch July 23, 2020 21:26

Implement Rayon traits (optional) #45

Implement Rayon traits (optional) #45

Uh oh!

Conversation

cuviper commented Nov 20, 2017

Uh oh!

bluss Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

bluss left a comment

Choose a reason for hiding this comment

Uh oh!

bluss Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

bluss commented Nov 22, 2017

Uh oh!

cuviper commented Nov 22, 2017

Uh oh!

bluss commented Nov 22, 2017

Uh oh!

bluss commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Nov 22, 2017

Uh oh!

vitiral commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Jan 3, 2018

Uh oh!

cuviper commented Feb 16, 2018

Uh oh!

bluss commented Feb 19, 2018

Uh oh!

cuviper commented Feb 19, 2018

Uh oh!

bluss commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ConnorGray commented Oct 29, 2018

Uh oh!

cuviper commented Oct 29, 2018

Uh oh!

ConnorGray commented Oct 29, 2018

Uh oh!

bluss commented Nov 21, 2018

Uh oh!

garro95 commented Apr 29, 2019

Uh oh!

cuviper commented Aug 20, 2019

Uh oh!

bluss left a comment

Choose a reason for hiding this comment

Uh oh!

cuviper commented Aug 20, 2019

Uh oh!

bluss commented Aug 20, 2019

Uh oh!

Uh oh!

bluss commented Nov 22, 2017 •

edited

Loading

vitiral commented Jan 3, 2018 •

edited

Loading

bluss commented Feb 21, 2018 •

edited

Loading