[libc++] `std::ranges::advance`: avoid unneeded bounds checks when advancing iterator #84126

jiixyj · 2024-03-06T06:59:02Z

Currently, the bounds check in std::ranges::advance(it, n, s) is done before n is checked. This results in one extra, unneeded bounds check.

Thus, std::ranges::advance(it, 1, s) currently is not simply equivalent to:

if (it != s) {
    ++it;
}

This difference in behavior matters when the check involves some "expensive" logic. For example, the == operator of std::istreambuf_iterator may actually have to read the underlying streambuf.

Swapping around the checks in the while results in the expected behavior.

llvmbot · 2024-03-06T06:59:30Z

@llvm/pr-subscribers-libcxx

Author: Jan Kokemüller (jiixyj)

Changes

Currently, the bounds check in std::ranges::advance(it, n, s) is done before n is checked. This results in one extra, unneeded bounds check.

Thus, std::ranges::advance(it, 1, s) currently is not simply equivalent to:

if (it != s) {
    ++it;
}

This difference in behavior matters when the check involves some "expensive" logic. For example, the == operator of std::istreambuf_iterator may actually have to read the underlying streambuf.

Swapping around the checks in the while results in the expected behavior.

Full diff: https://github.com/llvm/llvm-project/pull/84126.diff

1 Files Affected:

(modified) libcxx/include/__iterator/advance.h (+2-2)

diff --git a/libcxx/include/__iterator/advance.h b/libcxx/include/__iterator/advance.h
index 7959bdeae32643..296db1aaab6526 100644
--- a/libcxx/include/__iterator/advance.h
+++ b/libcxx/include/__iterator/advance.h
@@ -170,14 +170,14 @@ struct __fn {
     } else {
       // Otherwise, if `n` is non-negative, while `bool(i != bound_sentinel)` is true, increments `i` but at
       // most `n` times.
-      while (__i != __bound_sentinel && __n > 0) {
+      while (__n > 0 && __i != __bound_sentinel) {
         ++__i;
         --__n;
       }
 
       // Otherwise, while `bool(i != bound_sentinel)` is true, decrements `i` but at most `-n` times.
       if constexpr (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>) {
-        while (__i != __bound_sentinel && __n < 0) {
+        while (__n < 0 && __i != __bound_sentinel) {
           --__i;
           ++__n;
         }

jiixyj · 2024-03-06T07:01:27Z

The tests don't catch this at the moment. I'm thinking about how to test this. What do you think about adding a counter for operator== calls on stride_counting_iterator?

github-actions · 2024-03-09T10:25:36Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jiixyj · 2024-03-09T10:26:04Z

I added some tests that check that the equality comparison operator isn't called unnecessarily by adding another counter to stride_counting_iterator.

var-const

Thanks a lot for noticing and fixing this! Almost LGTM except a couple questions about the test.

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

var-const · 2024-03-12T06:18:40Z

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

+      assert(it.equals_count() == 0);
+    } else {
+      assert(it.equals_count() > 0);
+      assert(it.equals_count() == M || it.equals_count() == M + 1);


Question: why are there two possibilities?

The M case is the "normal" case where the iterator is incremented without the bound being "hit" (it may equal the bound though, but this last equals check shouldn't be executed).

The "M + 1" case happens when the advancing while loop is exited because of the bounds check, so the bound is "hit". Example: Calling std::ranges::advance(it, 999999, s) on a range { 42, 3, 4 } (size 3) will do 4 (3 + 1) bounds checks.

Thanks for explaining! In that case, we should either pass the expected number of comparisons as a parameter to this function, or pass a boolean parameter to indicate whether we expect the algorithm to hit the sentinel or not. Test cases should be simple so that we can be sure exactly what we're testing.

That makes perfect sense. I'll update the test accordingly.

var-const · 2024-03-12T06:19:47Z

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

@@ -213,6 +221,20 @@ constexpr bool test() {
    assert(i == iota_iterator{INT_MIN+1});
  }

+  // Check that we don't do an unneeded bounds check when decrementing a


Question: can you briefly explain why this case is special?

The existing tests for the "backwards" case seem to exclude bidirectional iterators:

llvm-project/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

Lines 191 to 199 in 1d296ec

{

// Note that we can only test ranges::advance with a negative n for iterators that

// are sized sentinels for themselves, because ranges::advance is UB otherwise.

// In particular, that excludes bidirectional_iterators since those are not sized sentinels.

int* expected = n > size ? range : range + size - n;

check_backward<random_access_iterator<int*>>(range, range+size, -n, expected);

check_backward<contiguous_iterator<int*>>( range, range+size, -n, expected);

check_backward<int*>( range, range+size, -n, expected);

}

So the tests didn't cover the part of std::ranges::advance where the iterator is decremented "one by one".

I'm not sure why the comment says that being a sized sentinel is a requirement -- at least in the current draft of the Standard, all I see is a requirement that the iterator and the sentinel are the same type. IIUC, if the comment were true, the new test case would actually be UB (because it calls advance with a negative n and a bidirectional iterator).

Assuming the comment is not correct, IMO the !sized_sentinel_for check creates more confusion than clarity. The bidirectional iterator concept doesn't require the type to be a sized sentinel for itself, and in tests we use "minimal" iterator types (types that only support the bare minimum operations required to satisfy the concept). So I think this would be clearer if we simply test with a bidirectional iterator and omit the static_asserts and the comment.

Yes, that comment does not seem to be correct.

My intention with the !sized_sentinel_for check was to not fall into this case: https://eel.is/c++draft/iterators#range.iter.op.advance-6.1, but instead test the other one: https://eel.is/c++draft/iterators#range.iter.op.advance-6.2.

But it should actually be possible to remove the new, "special case" test and instead improve the generic check_backward tests, right? Because as you mentioned, being sized_sentinel_for<It, It> is not actually a precondition for "advance negative n". In an improved check_backward test, one would only have to ensure the actual preconditions:

[bound, i) is a range

that It is a std::bidirectional_iterator

that S (the sentinel type) is the same as It

Then, one could instantiate check_backward with different iterator types that either model sized_sentinel_for<It, It> or don't. This way it would be possible to get coverage for both "range.iter.op.advance-6.1" and "range.iter.op.advance-6.2". Does this sound sensible to you?

OK, I've removed that misleading comment for the check_backward tests and adapted them to take bidirectional iterators. Then, I could also remove my "special case" test I had added, since the check_backward tests now cover the "decrement one by one" case as well.

@ldionne Just wanted to double-check with you regarding the comment in case we're missing something.

var-const · 2024-03-27T04:03:31Z

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

+      assert(it.equals_count() == 0);
+    } else {
+      assert(it.equals_count() > 0);
+      assert(it.equals_count() == M || it.equals_count() == M + 1);


Thanks for explaining! In that case, we should either pass the expected number of comparisons as a parameter to this function, or pass a boolean parameter to indicate whether we expect the algorithm to hit the sentinel or not. Test cases should be simple so that we can be sure exactly what we're testing.

var-const · 2024-03-27T04:16:56Z

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

@@ -213,6 +221,20 @@ constexpr bool test() {
    assert(i == iota_iterator{INT_MIN+1});
  }

+  // Check that we don't do an unneeded bounds check when decrementing a


I'm not sure why the comment says that being a sized sentinel is a requirement -- at least in the current draft of the Standard, all I see is a requirement that the iterator and the sentinel are the same type. IIUC, if the comment were true, the new test case would actually be UB (because it calls advance with a negative n and a bidirectional iterator).

Assuming the comment is not correct, IMO the !sized_sentinel_for check creates more confusion than clarity. The bidirectional iterator concept doesn't require the type to be a sized sentinel for itself, and in tests we use "minimal" iterator types (types that only support the bare minimum operations required to satisfy the concept). So I think this would be clearer if we simply test with a bidirectional iterator and omit the static_asserts and the comment.

…rs as well

var-const

Almost LGTM! Let's figure out whether we can pass the number of comparisons as a parameter to test cases, and this will be ready to go from my perspective.

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

var-const

Thanks for addressing the feedback! Really minor comments -- I expect to LGTM in the next round.

...s/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp

…a struct for 'check_backwards'

var-const

LGTM, thanks a lot for preparing this patch and addressing all the feedback! The Android failure on the CI looks like an infrastructure issue -- I'll go ahead and merge this.

jiixyj requested a review from a team as a code owner March 6, 2024 06:59

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 6, 2024

ldionne added the ranges Issues related to `<ranges>` label Mar 7, 2024

var-const self-assigned this Mar 8, 2024

var-const reviewed Mar 12, 2024

View reviewed changes

var-const requested changes Mar 27, 2024

View reviewed changes

jiixyj requested a review from var-const March 29, 2024 10:06

jiixyj added 7 commits March 29, 2024 11:31

skip unneeded checks against '__bound_sentinel' when advancing iterator

1f82b8f

add tests

8a2459d

apply clang-format

e07b579

remove unneeded check

f23336a

test for the extra bounds check iff 'n > M'

2a3ea3b

adapt 'check_backward' test so that it can test bidirectional iterato…

61c0029

…rs as well

apply git-clang-format

0be3474

jiixyj force-pushed the ranges-advance-swap-checks-around branch from ad861c8 to 0be3474 Compare March 29, 2024 10:32

var-const requested changes Mar 31, 2024

View reviewed changes

simplify test logic by passing in expected results

28fc7b7

jiixyj requested a review from var-const April 1, 2024 10:39

var-const requested changes Apr 1, 2024

View reviewed changes

jiixyj added 2 commits April 2, 2024 08:48

use 'int' instead of 'ptrdiff_t' for expected counts, wrap them into …

6af34a3

…a struct for 'check_backwards'

also test 'n == 0' case in 'check_backward'

55ef2f0

jiixyj requested a review from var-const April 2, 2024 07:05

var-const approved these changes Apr 2, 2024

View reviewed changes

var-const merged commit 84ae8cb into llvm:main Apr 2, 2024

	{
	// Note that we can only test ranges::advance with a negative n for iterators that
	// are sized sentinels for themselves, because ranges::advance is UB otherwise.
	// In particular, that excludes bidirectional_iterators since those are not sized sentinels.
	int* expected = n > size ? range : range + size - n;
	check_backward<random_access_iterator<int*>>(range, range+size, -n, expected);
	check_backward<contiguous_iterator<int*>>( range, range+size, -n, expected);
	check_backward<int*>( range, range+size, -n, expected);
	}

[libc++] std::ranges::advance: avoid unneeded bounds checks when advancing iterator #84126

[libc++] std::ranges::advance: avoid unneeded bounds checks when advancing iterator #84126

Uh oh!

Conversation

jiixyj commented Mar 6, 2024

Uh oh!

llvmbot commented Mar 6, 2024

Uh oh!

jiixyj commented Mar 6, 2024

Uh oh!

github-actions bot commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiixyj commented Mar 9, 2024

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[libc++] `std::ranges::advance`: avoid unneeded bounds checks when advancing iterator #84126

[libc++] `std::ranges::advance`: avoid unneeded bounds checks when advancing iterator #84126

github-actions bot commented Mar 9, 2024 •

edited

Loading