added by_blocks method #831

wagnerf42 · 2021-03-02T09:54:35Z

hi,

here is the code for blocks. in draft request like we discussed last time.

cuviper

I think this is a nice and powerful abstraction, but I also think we could make it easier for cases we expect to be common. Perhaps there could be a couple helpers like:

    // double each time, like your example, but this name is so-so
    fn by_doubling_blocks(self, first: usize) -> ByDoublingBlocks<I>; 

    // always use the same size, like `by_blocks(repeat(size))`
    fn by_uniform_blocks(self, size: usize) -> ByUniformBlocks<I>;

Those ideas could still use the same BlocksCallback internally.

cuviper · 2021-03-19T16:06:29Z

src/iter/blocks.rs

+        // now we loop on each block size
+        while remaining_len > 0 && !consumer.full() {
+            // we compute the next block's size
+            let size = self.sizes.next().unwrap_or(std::usize::MAX);


There's a design choice here when sizes.next() returns None, with at least three options:

Consume the remainder as one entire block, which is what you have now.

Run the remainder for side effects, without feeding it into our consumer. Skip does this with its skipped prefix to match the semantics of std::iter::Skip.

Ignore the remainder altogether, basically acting like the suffix of Take.

Did you consider this? I think behavior like Take might actually be a nice choice.

Another question is whether size == 0 should be allowed. At the very least, it would be a wasted split here, which could just be considered the user's poor choice. Or we could make that impossible by having S produce NonZeroUsize items, but that makes it more annoying to write the iterator.

Hmm. It seems quite surprising to me to ignore some of the elements. I would definitely not expect that. Ultimately, I think we want to offer users a choice of how to manage this -- another behavior I could imagine would be "go on using the last block size that got returned until we are complete".

I am wondering if we can express these semantics with some composed operators? I'd sort of like to say something like .by_blocks().skip_remainder() or something. We could just add inherent methods to the return type to allow users to change back and forth between those modes, no?

It seems to me that the user can express most of those semantics in the iterator itself:

current MAX behavior: my_iter.chain(iter::once(usize::MAX))

repeat behavior: my_iter.chain(iter::repeat(last))

my suggested Take behavior: just my_iter, and let its None be the end

run remainder without consuming: ???

We could just add inherent methods to the return type to allow users to change back and forth between those modes, no?

Yeah, that's possible too.

another might be asserting that there is no remainder

I think at minimum we should document these techniques, and i might prefer a convenient way to express them

if we only allow constant sizes and doubling sizes in the public interface then this choice about what to do when the size iterator ends is of private concern.
i used to have both versions, one like an implicit take when it runs out and the other one forcing to consume everything. so there is indeed something to choose here.

i would favor consuming it all since I happened to get some bugs missing some elements.

also, i have some questions like:

should i add unit tests ? where ?

should i add benches ? where ?

I would be in favor of only allowing the two options until we know there is demand for something more general. As for unit tests, we typically just add #[test] in the modules, I think? Alternatively, you can add tests into the tests directory but we don't usually do that. (Is this right, @cuviper?)

For benchmarks, we generally modify the rayon-bench project.

For tests, there are a few generic sanity checks to add: tests/clones.rs, tests/debug.rs, and src/compile_fail/must_use.rs. Beyond that, adding unit #[test] directly in the module is fine, or break into a tests submodule if there are a lot. Doc-test examples are also nice to serve both doc and testing roles.

cuviper · 2021-03-19T16:24:12Z

src/iter/mod.rs

+    /// we stop at the first block containing the searched data.
+    fn by_blocks<S: IntoIterator<Item = usize>>(self, sizes: S) -> ByBlocks<Self, S> {
+        ByBlocks::new(self, sizes)
+    }
    /// Collects the results of the iterator into the specified


tiny nit: please add a blank line between items like the two functions here, and also between the type and fn in your trait implementations.

sorry i just saw this comment. in the meantime i changed the file so i'm not too sure what you are referring to.

I'll add inline suggestions for it.

ok this is taking shape.

however there are some functions i had which are not included here.
i'm not too sure if we should add them at least in this pull request.

wagnerf42 · 2021-03-24T09:37:55Z

I think this is a nice and powerful abstraction, but I also think we could make it easier for cases we expect to be common. Perhaps there could be a couple helpers like:
    // double each time, like your example, but this name is so-so
    fn by_doubling_blocks(self, first: usize) -> ByDoublingBlocks<I>; 

    // always use the same size, like `by_blocks(repeat(size))`
    fn by_uniform_blocks(self, size: usize) -> ByUniformBlocks<I>;
Those ideas could still use the same BlocksCallback internally.

hi, sorry for the delay. that's a good idea. it's the only two patterns i was able to come up with for now.

wagnerf42 · 2021-03-29T08:50:32Z

hi, so I did the changes to by_doubling_blocks and by_uniform_blocks.

i also removed the initial block size argument in by_doubling_blocks to make it more transparent to the user. tell me if you think it is not a good idea.

quick questions:

is it ok to return in both cases a ByBlocks or should I get two different types and hide the size iterator ?
is it ok if i do modifications to rayon_demo/src/find ? find_last and find_first are calling find_any

cuviper

These are the style nits I mentioned, just adding blank lines.

cuviper · 2021-03-29T21:44:30Z

src/iter/blocks.rs

+    type Output = C::Result;
+    fn callback<P: Producer<Item = T>>(mut self, mut producer: P) -> Self::Output {


Suggested change

type Output = C::Result;

fn callback<P: Producer<Item = T>>(mut self, mut producer: P) -> Self::Output {

type Output = C::Result;

fn callback<P: Producer<Item = T>>(mut self, mut producer: P) -> Self::Output {

cuviper · 2021-03-29T21:44:48Z

src/iter/blocks.rs

+    type Item = I::Item;
+    fn drive_unindexed<C>(self, consumer: C) -> C::Result


Suggested change

type Item = I::Item;

fn drive_unindexed<C>(self, consumer: C) -> C::Result

type Item = I::Item;

fn drive_unindexed<C>(self, consumer: C) -> C::Result

cuviper · 2021-03-29T21:45:19Z

src/iter/mod.rs

+    }
+    /// Normally, parallel iterators are recursively divided into tasks in parallel.


Suggested change

}

/// Normally, parallel iterators are recursively divided into tasks in parallel.

}

/// Normally, parallel iterators are recursively divided into tasks in parallel.

cuviper · 2021-03-29T21:45:34Z

src/iter/mod.rs

+    }
    /// Collects the results of the iterator into the specified


Suggested change

}

/// Collects the results of the iterator into the specified

}

/// Collects the results of the iterator into the specified

cuviper · 2021-03-29T21:48:44Z

is it ok to return in both cases a ByBlocks or should I get two different types and hide the size iterator ?

I think it would be better to hide those details.

is it ok if i do modifications to rayon_demo/src/find ? find_last and find_first are calling find_any

Yes, that's fine -- it's probably a good place to showcase the difference between these approaches.

nikomatsakis · 2021-04-01T15:00:54Z

src/iter/mod.rs

+    ///
+    /// This can have many applications but the most notable ones are:
+    /// - better performances with [`find_first()`]
+    /// - more predictable performances with [`find_any()`] or any interruptible computation


This is good -- I think it'd be good to make a section like

# When to use this method

and put this text in there, and maybe point at by_uniform_blocks as an alternative. I think users will be unclear when to sue it.

nikomatsakis · 2021-04-01T16:01:20Z

src/iter/mod.rs

+    /// This adaptor changes the default behavior by splitting the iterator into a **sequence**
+    /// of parallel iterators of given `blocks_size`.
+    /// The main application is to obtain better
+    /// memory locality (especially if the reduce operation re-use folded data).


This is good -- I think it'd be good to make a section like

# When to use this method

and put this text in there, and maybe point at by_uniform_blocks as an alternative. I think users will be unclear when to use it.

wagnerf42 · 2021-05-16T14:50:29Z

some more things which can be done:

add a sequential fold or try_fold method as argument to by_blocks (well, in a new method).
this way we could try_fold sequentially the results reduced in each block.
it would actually be needed to have the best manual find_first implementation.
have another iterator size based on timings (like: one block every x duration)

i'm a bit worried about the combinations between block sizes and iterator choices because
we would have 2 or 3 iterator choices times 2-3 block sizes choices so that's up to 9 methods.

wagnerf42 · 2021-05-17T06:06:30Z

actually it would not work this way because we would need to know what the return type is and this info is not known until the call to reduce. i think it might still be possible to do it but i would need to modify the Folder trait. if we add a seq_consume for consuming the blocks results then i think we can add a seq_fold method in ParallelIterator registering in the consumer what the blocks folding method would be.

i'll give it a try

cuviper · 2021-05-17T22:40:15Z

src/iter/blocks.rs

+#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
+#[derive(Debug, Clone)]
+pub struct ExponentialBlocks<I>(
+    ByBlocks<I, std::iter::Successors<usize, fn(&usize) -> Option<usize>>>,


This only needs to store I, and then a ByBlocks can be created on the fly in drive_unindexed. That will also let it use the direct unnameable closure type, rather than forcing it to a function pointer.

cuviper · 2021-05-17T22:45:10Z

src/iter/blocks.rs

+/// [`IndexedParallelIterator`]: trait.IndexedParallelIterator.html
+#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
+#[derive(Debug, Clone)]
+pub struct UniformBlocks<I>(ByBlocks<I, std::iter::Repeat<usize>>);


Similarly, this really only needs to store I and blocks_size at first, but in this case it probably doesn't matter either way.

cuviper · 2024-02-10T00:50:02Z

@wagnerf42 did you ever try the suggestion I had about dropping the I parameters? Essentially making them lazier about that, IIRC. I also wonder if you were still exploring your own ideas, since this is still in Draft.

wagnerf42 · 2024-02-12T09:26:37Z

@wagnerf42 did you ever try the suggestion I had about dropping the I parameters? Essentially making them lazier about that, IIRC. I also wonder if you were still exploring your own ideas, since this is still in Draft.

well, i'm not too sure about what you mean. sorry it's been a while ago.
this adapter is one i use quite often and i'd rather get it done right.

- ByBlock is now private - indentation fixes

we could win big by adding more operations. it is super nice to have the sequential iterator on all reduced values of blocks. we could then use the dumb_find on each block and use the sequential find to find the first value. this would be the best algorithm. sadly we can't have it due to rayon's types encapsulation. the next best thing would be a try_fold_by_exponential_blocks method it would take a closure on Self producing the reduced values and a closure for the sequential try_fold

cuviper · 2024-02-15T01:29:16Z

If you have any comments on what I added, please let me know!

I plan to publish a new release soon, and it would be nice to include this.

cuviper reviewed Mar 19, 2021

View reviewed changes

nikomatsakis approved these changes Mar 19, 2021

View reviewed changes

cuviper reviewed Mar 29, 2021

View reviewed changes

nikomatsakis requested changes Apr 1, 2021

View reviewed changes

cuviper reviewed May 17, 2021

View reviewed changes

wagnerf42 mentioned this pull request Jan 4, 2022

Splitting strategy for find_first and find_last methods #908

Open

wagnerf42 and others added 10 commits February 12, 2024 20:00

added by_blocks method

1a49db3

added specific blocks sizes methods

2527cc3

tests for blocks operations

ff4056c

new public types for blocks

28743b3

- ByBlock is now private - indentation fixes

blocks: renamed doubling -> exponential

25f03e2

blocks: simplify the iterator types

93c2ca6

blocks: create the callback directly

be3a797

blocks: minor doc tweaks

2290ba0

blocks: assert block_size != 0

2cccfba

cuviper force-pushed the blocks branch from ddaf678 to 2cccfba Compare February 13, 2024 05:01

cuviper marked this pull request as ready for review February 13, 2024 05:19

blocks: extract the exponential size fn

4d2f0b6

cuviper enabled auto-merge February 27, 2024 01:17

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

Merged via the queue into rayon-rs:main with commit bacd468 Feb 27, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added by_blocks method #831

added by_blocks method #831

wagnerf42 commented Mar 2, 2021

cuviper left a comment

cuviper Mar 19, 2021

nikomatsakis Mar 19, 2021

cuviper Mar 22, 2021

nikomatsakis Mar 23, 2021

nikomatsakis Mar 23, 2021

wagnerf42 Mar 24, 2021

wagnerf42 Mar 24, 2021

nikomatsakis Mar 24, 2021

cuviper Mar 24, 2021

cuviper Mar 19, 2021

wagnerf42 Mar 29, 2021

cuviper Mar 29, 2021

wagnerf42 Mar 30, 2021

wagnerf42 commented Mar 24, 2021

wagnerf42 commented Mar 29, 2021

cuviper left a comment

cuviper Mar 29, 2021

cuviper Mar 29, 2021

cuviper Mar 29, 2021

cuviper Mar 29, 2021

cuviper commented Mar 29, 2021

nikomatsakis Apr 1, 2021

nikomatsakis Apr 1, 2021

wagnerf42 commented May 16, 2021

wagnerf42 commented May 17, 2021

cuviper May 17, 2021

cuviper May 17, 2021

cuviper commented Feb 10, 2024

wagnerf42 commented Feb 12, 2024

cuviper commented Feb 15, 2024

		type Output = C::Result;
		fn callback<P: Producer<Item = T>>(mut self, mut producer: P) -> Self::Output {

		type Item = I::Item;
		fn drive_unindexed<C>(self, consumer: C) -> C::Result

		}
		/// Normally, parallel iterators are recursively divided into tasks in parallel.

		}
		/// Collects the results of the iterator into the specified

added by_blocks method #831

added by_blocks method #831

Conversation

wagnerf42 commented Mar 2, 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

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 commented Mar 24, 2021

wagnerf42 commented Mar 29, 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

cuviper commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagnerf42 commented May 16, 2021

wagnerf42 commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Feb 10, 2024

wagnerf42 commented Feb 12, 2024

cuviper commented Feb 15, 2024