Descendants #836

wagnerf42 · 2021-03-24T11:04:38Z

hi,

this is an equivalent of the split function but for tree like patterns.
it solves an issue that was posted recently and i could also use it with some of my code.

tell me if you think it is worth inclusion for you. if so i can add some more tests and benches.

nikomatsakis

This seems definitely useful to me. I feel like an awful lot of parallel patterns wind up descending over trees.

nikomatsakis · 2021-03-24T19:31:31Z

src/iter/descendants.rs

+///    assert_eq!(v, vec![3, 10, 14, 18]);
+///    ```
+///
+pub fn descendants<S, B, I>(root: S, breed: B) -> Descendants<S, B, I>


I wonder about a different name, like iter::walk_tree or something?

cuviper · 2021-03-24T22:57:24Z

Do I understand correctly that this makes no guarantees about traversal order?

Of course parallel execution is unordered, but it's still visible in fold and reduce. I think it would be a lot stronger if we could specify the traversal and hold to that. It could even be generic pre/in/post-order if the user provided two closures, or one closure returning two things, so they choose what is logically traversed before or after the current item. Then examples like your tree could collect in the desired order, rather than sorting afterward.

I'm sure that would make the implementation harder though...

wagnerf42 · 2021-03-29T08:53:35Z

Do I understand correctly that this makes no guarantees about traversal order?

Of course parallel execution is unordered, but it's still visible in fold and reduce. I think it would be a lot stronger if we could specify the traversal and hold to that. It could even be generic pre/in/post-order if the user provided two closures, or one closure returning two things, so they choose what is logically traversed before or after the current item. Then examples like your tree could collect in the desired order, rather than sorting afterward.

I'm sure that would make the implementation harder though...

i'll take a look. note that it's not for binary trees but for arbitrary degrees so the only meaningful orders are pre and post

wagnerf42 · 2021-03-30T13:35:47Z

the ordering question was interesting.
actually i need to bench the sequential part (several algorithms in mind).
maybe i would do two functions walk_tree_prefix and walk_tree_postfix because they have very distinctive properties.
i might take a few days for that.

wagnerf42 · 2021-03-31T13:36:54Z

hi, so i did two functions with two different orderings.
prefix and postfix (order between children also differs in the two functions)
i benchmarked quite some number of variants and stayed with the fastest ones.
it is actually pretty fast. you can try the tree example.

there is an overhead for the prefix order because breed borrows the state so we must consume the whole iterator before consuming s and then loop again on the same data towards the children.

i'd like to have some feedback at this point if you can.
if that's ok for you then i'll add some tests for arbitrary degrees and the debug+clone tests

nikomatsakis

This looks good to me. I made some documentation suggestions. I think we should introduce a walk_tree that simply calls walk_tree_postfix, so that people have something they can use that does not guarantee ordering.

src/iter/walk_tree.rs

wagnerf42 · 2021-04-12T09:14:50Z

thanks for all your suggestions. it definitely is much cleaner now.

nikomatsakis

Looks great! One last nit.

src/iter/walk_tree.rs

cuviper · 2021-04-13T19:04:40Z

src/iter/walk_tree.rs

+/// which guarantees a postfix order.
+/// If you don't care about ordering, you should use [`walk_tree`],
+/// which will use whatever is believed to be fastest.
+/// Between siblings, children are reduced in reverse order -- that is, the children that returned last are reduced first.


The reverse order seems really bizarre -- is that necessary? As a user, I would expect a "normal" pre-order traversal of a binary tree to first visit the node, then the left children, then the right children.

hi, this is the fastest version. since it is a prefix order we need to consume the father before its children. but we need to borrow the father to iterate on children. this means we first need to extend the stack with the children.
because it's a vec the last pushed will be the first poped and this reverses children's order.

other options are:

use a more complex struct and some unsafe.

require the iterator to be double ended

first collect the iterator into a vec and then extend the stack in the right order

to me the only acceptable option is the third one. it comes with a performance hit though.

if you think it would be better to have the standard order then i can re-implement this version.

ok, so i did a quick bench (the one in rayon_demo)
this is the current version:
sum: 382us collect: 683us
this is the version with the "fixed" ordering:
sum: 705us collect: 950us

That speed difference seems pretty significant to me

first collect the iterator into a vec and then extend the stack in the right order

You could do this in-place by noting the previous length, extending, then reversing the new tail slice. That's still some added cost, but at least it doesn't require a new allocation.

i did it like this :

self.reorder_buffer.extend((self.breed)(&e)); self.to_explore.extend(self.reorder_buffer.drain(..).rev());

just to be clear, i do understand there is a choice to be made here.
the overhead is between 10 and 15 nanoseconds per element. this is not zero cost but not excessive.
if you ask me i would favor the current (reversed) order because the algorithm is "natural" this way. user can also reverse their iterators to get to the other order. however i do see why the other option also makes sense.

in the end, choose whatever you want and let me know.

Ideally, internal algorithm details should not be so visible in the public API, especially since those details might want to change in the future. I want to aim for what is most "natural" to the user first, without exposing details like "well actually you probably want to reverse this." That's even evident in your tree_prefix_collect benchmark where you have an explanatory comment, "large indices to the left, small to the right", being different than the others.

To that end, I think DoubleEndedIterator might be reasonable, although you immediately dismissed that. The user can provide the children in their natural order, matching the visitation order, but we'll have the flexibility to internally iterate in reverse. Are there realistic use-cases that you think could not be double-ended?

Of course there are plenty of non-DE iterators, but I'm asking for a real data structure that could not operate this way, especially since you say "users can also reverse their iterators." If they can so easily reverse, then DE should be no issue; if they can't, then I think we won't be serving them well with an algorithmically-reversed order.

so i did the switch to double ended iterators.
the non double ended iterators i use from time to time are scan and successors but i don't really see how they would be used here.

I think we now just need to update (or remove) this line about reversal.

src/iter/walk_tree.rs

wagnerf42 · 2021-04-16T11:00:33Z

i added a bit more tests with higher degrees

nikomatsakis

This is looking pretty good to me.

wagnerf42 · 2021-04-26T11:20:38Z

do i need to do something more ? i'm in no hurry but just tell me if you want something from me.

nikomatsakis · 2021-04-27T23:08:24Z

I'm satisfied -- @cuviper ?

cuviper

There's some minor cleanup, but otherwise I'm satisfied.

Please also take the PR out of draft state if you think it's ready.

cuviper · 2021-05-17T22:08:44Z

src/iter/walk_tree.rs

+/// which guarantees a postfix order.
+/// If you don't care about ordering, you should use [`walk_tree`],
+/// which will use whatever is believed to be fastest.
+/// Between siblings, children are reduced in reverse order -- that is, the children that returned last are reduced first.


I think we now just need to update (or remove) this line about reversal.

cuviper · 2021-05-17T22:14:42Z

src/iter/walk_tree.rs

+    S: Send,
+    B: Fn(&S) -> I + Send + Sync,
+    IT: DoubleEndedIterator<Item = S>,
+    I: IntoIterator<Item = S, IntoIter = IT> + Send,


I think we can remove IT as a public API parameter by constraining I::IntoIter: Double... instead.

Should we also require I::IntoIter: Send? We consume it immediately now, but might there be a future where we'd want to be more lazy about that?

i'm not sure i get it. what would send allow us to do ? IT would not be a parallel iterator so it would just mean using another thread. I is already Send.
so i'm not getting what this would potentially allow us to do and why I being Send is not enough

I: Send doesn't actually give us much, only that after getting such a return value of B, we could hold and send that I value to other threads before calling into_iter(). Once that's called, we now have the IntoIter type, which we might want to lazily pull items out one at a time.

For example, suppose to_explore were instead a Vec<Either<S, IntoIter>>, storing either unvisited nodes or their unprocessed (or partially processed) brood. We would need IntoIter: Send to be able to split this between threads.

Maybe we don't need to do this, especially if we don't expect there to be very many children most of the time.

Actually, I: Send is only necessary right now because you have that PhantomData<I>, but I don't think you actually need that. The structs themselves don't need the I parameter at all, only the impl constraints.

cuviper · 2021-05-17T22:16:31Z

src/iter/walk_tree.rs

+where
+    S: Send,
+    B: Fn(&S) -> I + Send + Sync,
+    I: IntoIterator<Item = S> + Send,


Same question about maybe I::IntoIter: Send.

src/iter/walk_tree.rs

cuviper · 2021-05-17T22:31:55Z

src/iter/walk_tree.rs

+where
+    S: Send,
+    B: Fn(&S) -> I + Send + Sync,
+    I: IntoIterator<Item = S> + Send,


Same question about maybe I::IntoIter: Send.

nikomatsakis · 2021-05-21T14:11:44Z

@wagnerf42 should we take the PR out of draft state?

nikomatsakis · 2021-05-21T14:12:07Z

you gotta click that "ready for review" button:

or we can...

this commit adds the `descendants` function which is an equivalent of `split` for tree structured data.

* renaming descendants to walk_tree * preserving order -> this has a performance cost so i'll try to do some benches

sequential folding is recursive i'm not sure how to do it sequentially since this would imply a self borrowing struct (we need to store both the state and the iterator borrowing it)

postfix collect test + bench, removed deque only thing needed now is tests for n-ary trees

removed vectors allocations + no recursion

changed order between siblings for that

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

- prefix order is reversed, we now required double ended iterators - tests and benches updated accordingly - two more tests for flat trees - removed unneeded malloc in task splitting

tests for graphs with higher degrees

- renamed breed to children_of - doc cleanup - using consume_iter

Otherwise we'll be under-constrained, unable to actually make a choice between the implementations. (Even though we think the faster one doesn't need that right now.)

cuviper · 2024-02-10T00:47:56Z

Hi @wagnerf42! Sorry that it's been a while. I took the liberty of rebasing your PR and making a few of those tweaks I had suggested way back in my last review. Please take a look, and if you're happy with this then I think we should merge it!

wagnerf42 · 2024-02-12T09:15:23Z

hi, well it seems fine for me.

cuviper · 2024-02-13T01:43:14Z

Thanks!

nikomatsakis approved these changes Mar 24, 2021

View reviewed changes

nikomatsakis requested changes Mar 31, 2021

View reviewed changes

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

nikomatsakis requested changes Apr 13, 2021

View reviewed changes

src/iter/walk_tree.rs Show resolved Hide resolved

cuviper reviewed Apr 13, 2021

View reviewed changes

cuviper reviewed Apr 14, 2021

View reviewed changes

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

src/iter/walk_tree.rs Outdated Show resolved Hide resolved

nikomatsakis approved these changes Apr 19, 2021

View reviewed changes

cuviper reviewed May 17, 2021

View reviewed changes

wagnerf42 marked this pull request as ready for review May 21, 2021 14:22

frederic wagner and others added 12 commits February 9, 2024 15:20

generic tree parallel iterator

9ca41b1

this commit adds the `descendants` function which is an equivalent of `split` for tree structured data.

descendants: fix for doc

97481b0

walk_tree

05eed2a

* renaming descendants to walk_tree * preserving order -> this has a performance cost so i'll try to do some benches

walk tree simplification

f6bc3eb

walk_tree: two functions for two orders

a07519f

sequential folding is recursive i'm not sure how to do it sequentially since this would imply a self borrowing struct (we need to store both the state and the iterator borrowing it)

walk_tree: clean + bench

e7c7481

postfix collect test + bench, removed deque only thing needed now is tests for n-ary trees

faster prefix walk tree

b56ae42

removed vectors allocations + no recursion

faster walk_tree_prefix

e3701d5

changed order between siblings for that

tree walks : documenting orders

4f214bb

Update src/iter/walk_tree.rs

ef06d79

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

Update src/iter/walk_tree.rs

d6fc8cc

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

Update src/iter/walk_tree.rs

33adf73

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

wagnerf42 and others added 12 commits February 9, 2024 15:20

Update src/iter/walk_tree.rs

1be2b13

Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

added walk_tree + doc fixes

2db07ef

benches for tree walks

f38e169

walk_tree: better documentation

d616a7c

walktree : prefix double ended iter

5f97d07

- prefix order is reversed, we now required double ended iterators - tests and benches updated accordingly - two more tests for flat trees - removed unneeded malloc in task splitting

walk_tree: more tests

4903d53

tests for graphs with higher degrees

walk tree: minor fixes

8ce2bdf

- renamed breed to children_of - doc cleanup - using consume_iter

walk_tree: reformat documentation

3bf4ae7

walk_tree: require doubled-ended for the unordered version

8256618

Otherwise we'll be under-constrained, unable to actually make a choice between the implementations. (Even though we think the faster one doesn't need that right now.)

walk_tree: drop I parameters from the types

5f708cc

walk_tree: don't require I: Send

306748c

walk_tree: use mem::take

96b365c

cuviper force-pushed the descendants branch from dd2414a to 96b365c Compare February 10, 2024 00:46

cuviper added this pull request to the merge queue Feb 13, 2024

Merged via the queue into rayon-rs:main with commit 46c49e6 Feb 13, 2024
4 checks passed

Descendants #836

Descendants #836

Conversation

wagnerf42 commented Mar 24, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Mar 24, 2021

wagnerf42 commented Mar 29, 2021

wagnerf42 commented Mar 30, 2021

wagnerf42 commented Mar 31, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

wagnerf42 commented Apr 12, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagnerf42 Apr 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagnerf42 Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagnerf42 commented Apr 16, 2021 • edited

nikomatsakis left a comment

Choose a reason for hiding this comment

wagnerf42 commented Apr 26, 2021

nikomatsakis commented Apr 27, 2021

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 21, 2021

nikomatsakis commented May 21, 2021

cuviper commented Feb 10, 2024

wagnerf42 commented Feb 12, 2024

cuviper commented Feb 13, 2024

wagnerf42 Apr 14, 2021 •

edited

wagnerf42 Apr 15, 2021 •

edited

wagnerf42 commented Apr 16, 2021 •

edited