Carefully remove bounds checks from some chunk iterator functions

[thomcc]

So, I was writing code that requires the equivalent of rchunks(N).rev() (which isn't the same as forward chunks(N) — in particular, if the buffer length is not a multiple of N, I must handle the "remainder" first).

I happened to look at the codegen output of the function (I was actually interested in whether or not a nested loop was being unrolled — it was), and noticed that in the outer rchunks(n).rev() loop, LLVM seemed to be unable to remove the bounds checks from the iteration: https://rust.godbolt.org/z/Tnz4MYY8f (this panic was from the split_at in RChunks::next_back).

After doing some experimentation, it seems all of the next_back in the non-exact chunk iterators have the issue: (Chunks::next_back, RChunks::next_back, ChunksMut::next_back, and RChunksMut::next_back)...

Even worse, the forward rchunks iterators sometimes have the issue as well (... but only sometimes). For example https://rust.godbolt.org/z/oGhbqv53r has bounds checks, but if I uncomment the loop body, it manages to remove the check (which is bizarre, since I'd expect the opposite...). I suspect it's highly dependent on the surrounding code, so I decided to remove the bounds checks from them anyway. Overall, this change includes:

• All next_back functions on the non-Exact iterators (e.g. R?Chunks(Mut)?).
• All next functions on the non-exact rchunks iterators (e.g. RChunks(Mut)?).

I wasn't able to catch any of the other chunk iterators failing to remove the bounds checks (I checked iterations over r?chunks(_exact)?(_mut)? with constant chunk sizes under -O3, -Os, and -Oz), which makes sense, since these were the cases where it was harder to prove the bounds check correct to remove...

In fact, it took quite a bit of thinking to convince myself that using unchecked_ here was valid — so I'm not really surprised that LLVM had trouble (although compilers are slightly better at this sort of reasoning than humans). A consequence of that is the fact that the // SAFETY comment for these are... kinda long...

---

I didn't do this for, or even think about it for, any of the other iteration methods; just next and next_back (where it mattered). If this PR is accepted, I'll file a follow up for someone (possibly me) to look at the others later (in particular, nth/nth_back looked like they had similar logic), but I wanted to do this now, as IMO next/next_back are the most important here, since they're what gets used by the iteration protocol.

---

Note: While I don't expect this to impact performance directly, the panic is a side effect, which would otherwise not exist in these loops. That is, this could prevent the compiler from being able to move/remove/otherwise rework a loop over these iterators (as an example, it could not delete the code for a loop whose body computes a value which doesn't get used).

Also, some like to be able to have confidence this code has no panicking branches in the optimized code, and "no bounds checks" is kinda part of the selling point of Rust's iterators anyway.

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[the8472]

taking over reviews for this one

r? @the8472

[JohnCSimon]

Ping from triage:
@thomcc - Can you please post the status of this PR?

[thomcc]

Yes, but probably not until the weekend

[thomcc]

Current status: The code works fine, and I've tried to remove the repetition from the safety comments, which is probably¹ for the best.

Anyway, I guess it can be reviewed now? I don't know that I can improve the safety comments further, but hopefully it's enough? That said, I'm willing to apply suggestions if they're suitably concrete².

Footnotes

1. They're arguably less precise now — you can't directly apply Chunks::next_back to e.g. ChunksMut::next_back — but it's probably good enough in practice for it to be clear why it's safe.

   I didn't do this initially because I was worried about things varying, but maintaining all these nearly-equivalent-but-slightly-different safety comments would suck, and it's not exactly clear how to pull it out into a common place (they're all necessarily slightly different for similar reasons to why the iterators can't reuse eachother internally). ↩

2. For example, I'm not exactly sure what to do with a suggestion to use mathematical notation (sorry). ↩

[the8472]

The comment length looks manageable now and the contents are ok too.

I looked over the existing tests and it appears that there are no explicit tests for next/next_back that would cover all branches. Maybe the tests for count() used to in the forward direction but there has been a separate implementation for that for quite some time now.

[the8472]

@JohnCSimon this should be waiting-on-author.

To make my previous comment more explicit, since this adds unsafe code it would be good to have test-coverage for it and the existing tests don't look like they cover it entirely.
Specifically Chunks::next_back returning Some doesn't appear to be covered, the None case is covered by test_chunks_nth_back.

[thomcc]

Ah, right, I forgot about this. I'll get to it later this weekend.

[the8472]

@thomcc ping, long weekend? 😄

[thomcc]

😅 I'll try to get to it when I find some time. Busy lately, unfortunately.

[thomcc]

Terribly sorry about the delay, I've added tests. They should cover both the exact size and remainder¹ cases, for all functions that I changed.

Footnotes

1. Well, you know what I mean — I didn't the change _exact fns, so I guess technically there's no remainder. ↩

[thomcc]

Let's try....

@rustbot ready

(Yay, it worked)

[the8472]

@bors r+ rollup=never

I'm not seeing much chunks use in the compiler itself but let's skip rollup just in case some lib uses it.

[bors]

📌 Commit 9c62455 has been approved by the8472

[bors]

⌛ Testing commit 9c62455 with merge 547f2ba...

[bors]

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 547f2ba to master...

[rust-timer]

Finished benchmarking commit (547f2ba): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression