impl IntoParallelIterator for Range<char> #771

CAD97 · 2020-06-18T03:32:09Z

Closes #770

Implements both unindexed and indexed parallel iteration for Range<char> and RangeInclusive<char>.

cuviper · 2020-06-18T20:35:23Z

I'm not so concerned about 16-bit usize for rayon -- the only such target built into rust is msp430-none-elf, which has core only.

src/range.rs

CAD97 · 2020-06-18T21:30:59Z

I've updated the PR to just be for Range<char> currently, which works already. The tests only fail because I added a smoke test for correctness across the surrogate range that tests against the std impl, which isn't stable until 1.45.

RangeInclusive cannot be implemented in the same manner, as RangeInclusive<u32> is not ExactSizeIterator, so the chain isn't IndexedParallelIterator. (I'm working on doing so only using Range<u32> right now.)

cuviper · 2020-06-18T21:33:37Z

RangeInclusive cannot be implemented in the same manner, as RangeInclusive<u32> is not ExactSizeIterator, so the chain isn't IndexedParallelIterator. (I'm working on doing so only using Range<u32> right now.)

Yeah, end_char as u32 + 1 will never overflow, so Range<u32> should be fine.

CAD97 · 2020-06-18T21:35:35Z

Alright, this should be correctly functional now. (Just that the tests require rustc 1.45 which is currently beta. (I can fix this by just comparing against a constant test case.))

CAD97 · 2020-06-18T21:44:39Z

The tests before 56ef498 are more obviously correct (as they compare the rayon and std implementations), but said commit compares them against a provided vector...

but the impl requires RangeInclusive<char>: Iterator anyway so that we can get the bounds and whether the iterator is exhausted.

So RangeInclusive<char>: IntoParallelIterator seems to need to be gated behind RangeInclusive<char>: Iterator, unlike the half-open case.

cuviper · 2020-06-18T21:41:44Z

src/range_inclusive.rs

+    where
+        C: Consumer<Self::Item>,
+    {
+        if let Some((start, end)) = self.bounds() {


Hmm, bounds() won't work until Rust 1.45...

FWIW, I was also looking at that function in godbolt, and it works a lot better for char this way:

if range.size_hint().0 > 0 { Some((*range.start(), *range.end())) } else { None }

https://rust.godbolt.org/z/8FrRRW

But it does require that we rely on size_hint() to be correct (like TrustedLen). I guess this code shouldn't be a hotspot anyway, so it probably doesn't matter.

The correct way to do it would be to use .is_empty() if/when it stabilizes.

That can be optimized in a followup if necessary/desired, I suppose. I don't think it would be incorrect to use size_hint here, so long as there's a note about relying on TrustedLen-like semantics for correctness (but hopefully not safety?).

Side note: oof, there's a unwrap/expect panic still in there. I guess I need to go tweak the impls again to see if I can remove that (as it should be completely unreachable, and if it isn't, that's definitely a bug.)

Yep, the generated asm is a lot better when I add in the missed _unchecked to next_back: https://rust.godbolt.org/z/9dviLq

We already have it for next because I spotted the missed optimization out of the unreachable panicking code, but for some reason I didn't think to check reverse iteration for the same thing.

I don't think it would be incorrect to use size_hint here, so long as there's a note about relying on TrustedLen-like semantics for correctness (but hopefully not safety?).

There's no safety problem, just start() and end() are unspecified when empty.

cuviper · 2020-06-18T21:49:39Z

So RangeInclusive<char>: IntoParallelIterator seems to need to be gated behind RangeInclusive<char>: Iterator, unlike the half-open case.

You can add a test to the build script to get a cfg.

CAD97 · 2020-06-18T21:52:40Z

You can add a test to the build script to get a cfg.

I got that in before you suggested it 🙃

Manually tested that this does engage and work on 1.45.

cuviper · 2020-06-18T22:09:17Z

For the other RangeInclusive impls we have a convert! macro which hides the code duplication -- maybe you could add a convert_char! for your case? You only have two places each, but it's still fairly complicated code.

CAD97 · 2020-06-18T22:25:12Z

Added a macro to deduplicate; I think this is what you were suggesting?

cuviper

I think it's good, but I'd like to wait for working Range<char> to actually reach stable, especially since we're not guarding the implementation of rayon::range::Iter<char>.

cuviper · 2020-06-18T23:16:33Z

And since I can't help tinkering, here's another option for bounds without Iterator:

    let start = *range.start();
    let end = *range.end();
    if start <= end && (start..=end) == *range {
        Some((start, end))
    } else {
        None
    }

... assuming Eq captures the private exhausted state, I think this would be OK. It actually looks better for char than for i32! https://rust.godbolt.org/z/TT2DBm

CAD97 · 2020-06-19T01:11:44Z

And since I can't help tinkering, here's another option for bounds without Iterator:

Here's a comparison of a bunch of the proposed options, with next_back fixed to optimize out the unreachable overflow check: https://rust.godbolt.org/z/F25_Ej

TL;DR: it looks to my untrained eyes that any of the proposed options are roughly equivalent. (Note for _eq2: Some no longer implies there is iteration remaining, but could construct "reverse ranges" when given a reverse range. This will, however, still produce a correctly empty iterator.) I believe the char case looks better because of how niches interact with the return value (and thus the difference should disappear when inlined).

CAD97 · 2020-06-19T01:17:26Z

The commit just pushed uses start <= end && range == (start..=end) to check that using the bounds is still correct. (The start <= end check, though redundant for the char case, is in fact requisite for the .chain(once(end)) case. This wasn't caught by tests when I implemented it -- needed test? u32::MAX..=0 would yield u32::MAX without the start <= end check.)

EDIT: no, I need to go to bed, apparently. The code is definitely still correct with bounds returning (high, low)... because if low is the maximum value, then it's >= the high value, so the range is not exhausted.

So bounds() could be implemented with just the range == (start..=end) check, but I'll keep the start <= end check to avoid changing its semantics in this PR.

Waiting until iterable Range<char> is stable to merge this makes sense.

@cuviper

…nieu Use step_unchecked more liberally in range iter impls Without these `_unchecked`, these operations on iterators of `char` fail to optimize out the unreachable panicking condition on overflow. cc @cuviper rayon-rs/rayon#771 where this was discovered.

cuviper · 2020-07-21T19:39:30Z

I think we're good for this now, thanks!

bors r+

bors · 2020-07-21T19:46:39Z

Build succeeded:

CAD97 marked this pull request as draft June 18, 2020 05:15

cuviper reviewed Jun 18, 2020

View reviewed changes

src/range.rs Outdated Show resolved Hide resolved

CAD97 force-pushed the range-char branch from 5503a13 to b878116 Compare June 18, 2020 21:23

par_iter for Range<char>

a04c20a

CAD97 force-pushed the range-char branch from b878116 to a04c20a Compare June 18, 2020 21:29

par_iter for RangeInclusive<char>

4c99c56

CAD97 marked this pull request as ready for review June 18, 2020 21:35

Allow char iter tests to run on rusts before 1.45

56ef498

cuviper reviewed Jun 18, 2020

View reviewed changes

Gate par_iter for RangeInclusive<char>

794d130

Deduplicate char iteration with a macro

51513bc

cuviper approved these changes Jun 18, 2020

View reviewed changes

CAD97 force-pushed the range-char branch from 5e71d41 to 228e8dc Compare June 19, 2020 01:14

Ungate par_iter for RangeInclusive<char>

8f37302

CAD97 force-pushed the range-char branch from 228e8dc to 8f37302 Compare June 19, 2020 01:15

CAD97 mentioned this pull request Jun 19, 2020

Use step_unchecked more liberally in range iter impls rust-lang/rust#73490

Merged

bors bot merged commit 97b7e34 into rayon-rs:master Jul 21, 2020

CAD97 deleted the range-char branch July 22, 2020 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl IntoParallelIterator for Range<char> #771

impl IntoParallelIterator for Range<char> #771

CAD97 commented Jun 18, 2020 •

edited

Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 •

edited

Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 •

edited

Loading

CAD97 commented Jun 18, 2020

cuviper Jun 18, 2020

CAD97 Jun 18, 2020

CAD97 Jun 18, 2020

CAD97 Jun 18, 2020

cuviper Jun 18, 2020

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 •

edited

Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020

cuviper left a comment

cuviper commented Jun 18, 2020

CAD97 commented Jun 19, 2020 •

edited

Loading

CAD97 commented Jun 19, 2020 •

edited

Loading

cuviper commented Jul 21, 2020

bors bot commented Jul 21, 2020

impl IntoParallelIterator for Range<char> #771

impl IntoParallelIterator for Range<char> #771

Conversation

CAD97 commented Jun 18, 2020 • edited Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 • edited Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 • edited Loading

CAD97 commented Jun 18, 2020

cuviper Jun 18, 2020

Choose a reason for hiding this comment

CAD97 Jun 18, 2020

Choose a reason for hiding this comment

CAD97 Jun 18, 2020

Choose a reason for hiding this comment

CAD97 Jun 18, 2020

Choose a reason for hiding this comment

cuviper Jun 18, 2020

Choose a reason for hiding this comment

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020 • edited Loading

cuviper commented Jun 18, 2020

CAD97 commented Jun 18, 2020

cuviper left a comment

Choose a reason for hiding this comment

cuviper commented Jun 18, 2020

CAD97 commented Jun 19, 2020 • edited Loading

CAD97 commented Jun 19, 2020 • edited Loading

cuviper commented Jul 21, 2020

bors bot commented Jul 21, 2020

CAD97 commented Jun 18, 2020 •

edited

Loading

CAD97 commented Jun 18, 2020 •

edited

Loading

CAD97 commented Jun 18, 2020 •

edited

Loading

CAD97 commented Jun 18, 2020 •

edited

Loading

CAD97 commented Jun 19, 2020 •

edited

Loading

CAD97 commented Jun 19, 2020 •

edited

Loading