[libc++] Make drop_view::begin constant time (#72883) #72929

casavaca · 2023-11-21T00:40:26Z

As pointed out in #72883, the implementation only needs to return the
value of ranges::next and does not need to obtain the value through
ranges::advance, which causes it to have O(n) complexity in the case
of random-access-sized but non-common range.

Fixes #72883

llvmbot · 2023-11-21T00:40:57Z

@llvm/pr-subscribers-libcxx

Author: Hongyu Ouyang (casavaca)

Changes

as pointed out from the github issue #72883 :
> ... the implementation only needs to return the value of ranges::next
and does not need to obtain the value through ranges::advance, which will have
O(n) complexity in the case of random-access-sized but non-common range.

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

1 Files Affected:

(modified) libcxx/include/__ranges/drop_view.h (+2-1)

diff --git a/libcxx/include/__ranges/drop_view.h b/libcxx/include/__ranges/drop_view.h
index f10476f0011e739..2557d030fd61145 100644
--- a/libcxx/include/__ranges/drop_view.h
+++ b/libcxx/include/__ranges/drop_view.h
@@ -104,7 +104,8 @@ namespace ranges {
     constexpr auto begin() const
       requires random_access_range<const _View> && sized_range<const _View>
     {
-      return ranges::next(ranges::begin(__base_), __count_, ranges::end(__base_));
+      auto __dist = ranges::distance(__base_);
+      return ranges::begin(__base_) + std::min<range_difference_t<_View>>(__count_, __dist);
     }
 
     _LIBCPP_HIDE_FROM_ABI

EricWF · 2023-11-21T01:14:28Z

Please add tests or benchmarks of some sort.

hawkinsw · 2023-11-21T01:49:06Z

Are we missing another optimization here, too?

For the non-const version of begin(), it could be possible for __simple_view<_View> to be the only one of the three conditions to fail. That would get us to exactly the same place as the requirements for the const version of begin(), just called on a non-const drop_view. So, should there be an optimization here, too?

casavaca · 2023-11-21T03:55:07Z

Upon further investigation, I found that the library correctly implement O(1) behavior. The call sequence is

ranges::next --> ranges::advance (with sentinel) --> ranges::advance (without sentinel) --> random_access_iterator constexpr branch

So, the original behavior is correct, and I'm closing this pull request.

casavaca · 2023-11-21T04:00:02Z

Oops, I didn't pay attention to non-common range...

hawkinsw · 2023-11-21T04:15:09Z

Oops, I didn't pay attention to non-common range...

Which would get us to a position where

ranges::end(__base_) is of a different type than ranges::begin(__base_)
therefore, ranges::end(__base_) might not model random_access_iterator
therefore, it would not meet the constant-time-complexity requirements for std::ranges::next (see (4) at https://en.cppreference.com/w/cpp/iterator/ranges/next).

I just wanted to double check the reasoning. Thank you for doing this work!
Will

casavaca · 2023-11-21T07:29:14Z

OK. Hopefully I did it right this time. First time contributor here.

git clang-format
2 changes at drop_view.h are covered by 2 newly added test cases respectively.

libcxx/include/__ranges/drop_view.h

huixie90 · 2023-11-21T19:46:00Z

libcxx/include/__ranges/drop_view.h

@@ -90,6 +90,10 @@ namespace ranges {
      requires (!(__simple_view<_View> &&
                  random_access_range<const _View> && sized_range<const _View>))
    {
+      if constexpr (random_access_range<const _View> && sized_range<const _View>) {
+        auto __dist = ranges::distance(__base_);
+        return ranges::begin(__base_) + std::min<range_difference_t<_View>>(__count_, __dist);


Btw, please double check if this file has undef the min/max macro on the top

Not found anything similar. Besides, std::min is also used in other places of this file.

yes there is already push/pop macros in this file

huixie90 · 2023-11-21T19:48:50Z

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

+    static_assert(std::ranges::sized_range<const NonCommonView>);
+    std::ranges::drop_view dropView9(NonCommonView{{}, 0, &sentinel_cmp_calls}, 4);
+    assert(dropView9.begin() == globalBuff + 4);
+    assert(sentinel_cmp_calls == 0);


The tests under libcxx/std are used by other implementations too (e.g. MSVC STL). It is not required by the spec that it has to be called zero times. I would instead relax this check a bit. For example , calling begin in the loop and check that we is not grow linearly ?

Yep. I can relax the test by comparing if sentinel_cmp_calls is a constant, but I have a question:
Since this is not required by the standard, how should I add test like this? I mean, should we use
something like if defined(LIBCXX_ONLY_TESTS)? or other other way? @huixie90

For this one i think it would be good to keep it once you relaxed it as the constant time begin is required by the standard.

In general if you have Libc++ specific test, you can put it under test/libcxx

hewillk

I don't think explicitly specifying the type improves readability, ranges::distance already implies signed values.

hewillk · 2023-11-22T02:48:33Z

libcxx/include/__ranges/drop_view.h

@@ -90,6 +90,10 @@ namespace ranges {
      requires (!(__simple_view<_View> &&
                  random_access_range<const _View> && sized_range<const _View>))
    {
+      if constexpr (random_access_range<_View> && sized_range<_View>) {
+        range_difference_t<_View> __dist = ranges::distance(__base_);


Suggested change

range_difference_t<_View> __dist = ranges::distance(__base_);

const auto __dist = ranges::distance(__base_);

I was like why don't you use ranges::size here and that is exactly what you want to call since that is what is constrained on. then I realised that oh you want to call std::min on two signed types so you used ranges::distance just for converting the types. maybe
range_difference_t<_View> __dist = ranges::size(__base_);
is the actual intent of the code?

maybe
range_difference_t<_View> __dist = ranges::size(__base_);
is the actual intent of the code?

The standard does not guarantee that the above implicit conversion is well-formed. ranges::distance is always preferred over ranges::size when deal with iterators.

maybe
range_difference_t<_View> __dist = ranges::size(__base_);
is the actual intent of the code?

The standard does not guarantee that the above implicit conversion is well-formed. ranges::distance is always preferred over ranges::size when deal with iterators.

Only narrowing conversion can be explicit-only. IIUC if size_t -> difeerence_t is narrowing conversion, I think we have other problems. The problem of ranges::distance is that it is not for the purpose of calculating the size in constant time. It is a utility tool to calculate the size in case you cannot caluculate it in constant time, which is the wrong tool here imo. And I strongly against using ‘auto’ when dealing with difference type for the exact reason we are discussing: narrowing conversion.
If the spec does not say size -> difference_t cannot be narrowing, I’d prefer to use either parenthesis or static_cast to do the comversion

Only narrowing conversion can be explicit-only. IIUC if size_t -> difeerence_t is narrowing conversion, I think we have other problems.

The standard does not require the signedness of the return value of ranges::size. The only guarantee is that it is an integer-like type. So in theory, the types returned by ranges::size and ranges::distance can be unrelated and only be explicitly converted.

The problem of ranges::distance is that it is not for the purpose of calculating the size in constant time.

This is why the sized_range needs to be satisfied, isn't it? in which case ranges::distance is guaranteed to be constant-time by its definition.

Only narrowing conversion can be explicit-only. IIUC if size_t -> difeerence_t is narrowing conversion, I think we have other problems.

The standard does not require the signedness of the return value of ranges::size. The only guarantee is that it is an integer-like type. So in theory, the types returned by ranges::size and ranges::distance can be unrelated and only be explicitly converted.

The problem of ranges::distance is that it is not for the purpose of calculating the size in constant time.

This is why the sized_range needs to be satisfied, isn't it? in which case ranges::distance is guaranteed to be constant-time by its definition.

If you have a helper

Void helper() {
if (c)
F();
else
G();
}

When you definitely don’t want to call G(), why do you write

if(c)
helper()

Rather than call f() directly ?

Only narrowing conversion can be explicit-only. IIUC if size_t -> difeerence_t is narrowing conversion, I think we have other problems.

The standard does not require the signedness of the return value of ranges::size. The only guarantee is that it is an integer-like type. So in theory, the types returned by ranges::size and ranges::distance can be unrelated and only be explicitly converted.

The problem of ranges::distance is that it is not for the purpose of calculating the size in constant time.

This is why the sized_range needs to be satisfied, isn't it? in which case ranges::distance is guaranteed to be constant-time by its definition.

Anyway I think our discussion about coding style should not block this patch to be landed

If you have a helper

Void helper() { if (c) F(); else G(); }

When you definitely don’t want to call G(), why do you write

if(c) helper()

Rather than call f() directly ?

This example is not quite accurate because the return type of ranges::distance is what we want, which reduces the type conversion since the former has already been done for us. Using ranges::size requires an additional static_cast.

I believe the standard prefers using rangs::begin(r) + rangs::distance(r) rather than rangs::begin(r) + static_cast<range_difference_t<R>>(rangs::size(r)), given the former's heavy presence in the <ranges>'s wording. (which may be the reason why my first resolution in LWG 3717 was not accepted))

I am fine with both. But I think the spec is describing the behaviour and not necessarily the way one should code. e.g this particular function here the spec is simply describing what the result should be, “Returns” , and one should not use the exact wording for implementation . (I saw you create an LWG issue for this but lots of us think it is not a defect because the spec does not say “equivalent to”. But I agree this leads to buggy implementations )

hewillk · 2023-11-22T02:48:53Z

libcxx/include/__ranges/drop_view.h

@@ -104,7 +108,8 @@ namespace ranges {
    constexpr auto begin() const
      requires random_access_range<const _View> && sized_range<const _View>
    {
-      return ranges::next(ranges::begin(__base_), __count_, ranges::end(__base_));
+      range_difference_t<_View> __dist = ranges::distance(__base_);


Suggested change

range_difference_t<_View> __dist = ranges::distance(__base_);

const auto __dist = ranges::distance(__base_);

casavaca · 2023-11-23T09:06:43Z

@huixie90 Is there anything I need to do? do you want me to change range_difference_t<_View> __dist to const auto __dist?

casavaca · 2023-12-07T00:12:22Z

@huixie90 any update on this?

huixie90

LGTM with green CI

huixie90 · 2023-12-13T11:16:44Z

libcxx/include/__ranges/drop_view.h

@@ -104,7 +108,8 @@ namespace ranges {
    constexpr auto begin() const
      requires random_access_range<const _View> && sized_range<const _View>
    {
-      return ranges::next(ranges::begin(__base_), __count_, ranges::end(__base_));
+      range_difference_t<_View> __dist = ranges::distance(__base_);
+      return ranges::begin(__base_) + std::min(__count_, __dist);


super nit:
the type of (__count_) is range_difference_t<_View> const& and the type of (__dist) is range_difference_t<const _View> & and I am not sure range_difference_t<_View> and range_difference_t<const _View> have to be the same type.

For my own edification, I attempted to answer this question ... it looks like the type is stripped of const/&?

template <range _Rp> using range_difference_t = iter_difference_t<iterator_t<_Rp>>; // ... template <class _Ip> using iter_difference_t = typename conditional_t<__is_primary_template<iterator_traits<remove_cvref_t<_Ip> > >::value, incrementable_traits<remove_cvref_t<_Ip> >, iterator_traits<remove_cvref_t<_Ip> > >::difference_type;

For my own edification, I attempted to answer this question ... it looks like the type is stripped of const/&?

template <range _Rp> using range_difference_t = iter_difference_t<iterator_t<_Rp>>; // ... template <class _Ip> using iter_difference_t = typename conditional_t<__is_primary_template<iterator_traits<remove_cvref_t<_Ip> > >::value, incrementable_traits<remove_cvref_t<_Ip> >, iterator_traits<remove_cvref_t<_Ip> > >::difference_type;

I think this is unrelated. off course these traits needs to strip off cvref, but iterator_t<_Rp> and iterator_t<const _Rp> can well be two different types. (although one would argue that it is evil to let them have two different difference_type)

Good catch! I missed the nested iterator_t inside the definition of the range_difference_t.

And, you're right, that would be evil!

casavaca · 2023-12-14T00:24:51Z

Rebased on llvm/main. Also made stylish chanege to use const auto, similar to microsoft STL in
https://github.com/microsoft/STL/blob/cf1313c39169dc376761eddee23c5e408e01aaa9/stl/inc/ranges#L3359

Let's see if CI is happy.

huixie90 · 2023-12-21T08:14:37Z

Hi, all CI passed now. could you please rebase and resolve the conflicts? It should be just a formatting change that had the conflicts. Thank you for your contribution

The optimization is specifically for non common range, as pointed out from the github issue: > ... the implementation only needs to return the value of ranges::next and does not need to obtain the value through ranges::advance, which will have O(n) complexity in the case of random-access-sized but non-common range.

casavaca requested a review from a team as a code owner November 21, 2023 00:40

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 21, 2023

casavaca closed this Nov 21, 2023

casavaca reopened this Nov 21, 2023

casavaca force-pushed the main branch from ae09b38 to 081f76d Compare November 21, 2023 07:25

huixie90 reviewed Nov 21, 2023

View reviewed changes

casavaca requested a review from huixie90 November 21, 2023 23:21

hewillk reviewed Nov 22, 2023

View reviewed changes

ldionne assigned huixie90 Dec 12, 2023

huixie90 approved these changes Dec 13, 2023

View reviewed changes

casavaca force-pushed the main branch from b75fb8b to 0adb33f Compare December 14, 2023 00:23

ldionne changed the title ~~[libc++] optimization on ranges::drop_view::begin (#72883)~~ [libc++] Make drop_view::begin constant time (#72883) Dec 18, 2023

casavaca added 3 commits December 21, 2023 13:27

Update comment (remove zero width space)

e2ec7a4

rebase

9bdf7bb

casavaca force-pushed the main branch from a474180 to 9bdf7bb Compare December 21, 2023 21:32

huixie90 merged commit 37f9036 into llvm:main Dec 28, 2023

	range_difference_t<_View> __dist = ranges::distance(__base_);
	const auto __dist = ranges::distance(__base_);

[libc++] Make drop_view::begin constant time (#72883) #72929

[libc++] Make drop_view::begin constant time (#72883) #72929

Conversation

casavaca commented Nov 21, 2023 • edited by ldionne Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 21, 2023

Uh oh!

EricWF commented Nov 21, 2023

Uh oh!

hawkinsw commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casavaca commented Nov 21, 2023

Uh oh!

casavaca commented Nov 21, 2023

Uh oh!

hawkinsw commented Nov 21, 2023

Uh oh!

casavaca commented Nov 21, 2023

Uh oh!

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!

hewillk left a comment

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!

hewillk Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hewillk Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huixie90 Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casavaca commented Nov 23, 2023

Uh oh!

casavaca commented Dec 7, 2023

Uh oh!

huixie90 left a comment

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!

casavaca commented Dec 14, 2023

casavaca commented Nov 21, 2023 •

edited by ldionne

Loading

hawkinsw commented Nov 21, 2023 •

edited

Loading

hewillk Nov 23, 2023 •

edited

Loading

hewillk Nov 23, 2023 •

edited

Loading

huixie90 Nov 25, 2023 •

edited

Loading