Optimizations for unreachable sentinels by AdamBucior · Pull Request #1810 · microsoft/STL

AdamBucior · 2021-04-06T15:57:40Z

Enables optimizations for contiguous iterator + unreachable_sentinel_t ranges.

The only thing I'm concerned about is users overloading operator==(custom contiguous iterator, unreachable_sentinel_t) which doesn't always return false. Such code should definitely be forbidden by the standard but I'm not sure if it is.

miscco

I am a bit concerned about the additional derivations of _Copy_memcpy

Also I believe the _STL_ASSERTs should bee _STATIC_ASSERT(_Always_false, ...)

miscco · 2021-04-07T05:41:14Z

+                          && (_Is_sized1 || same_as<_Se1, unreachable_sentinel_t>) //
+#pragma warning(suppress : 6287) // Redundant code: the left and right subexpressions are identical
+                          &&(_Is_sized2 || same_as<_Se2, unreachable_sentinel_t>) //
+                          &&same_as<_Pj1, identity> && same_as<_Pj2, identity>) {


In the above case you put those constraints on top. I would prefer to keep them there but downgrade to is_same_v or _Same_impl

All of the ranges algorithm optimizations I've written already use same_as.

miscco · 2021-04-07T05:51:34Z

+    template <class _InIt, class _OutIt, typename _DistIt>
+    in_out_result<_InIt, _OutIt> _Copy_memcpy_distance(
+        _InIt _IFirst, _OutIt _OFirst, _DistIt _DFirst, _DistIt _DLast) noexcept {
+        const auto _IFirstPtr = _To_address(_IFirst);
+        const auto _OFirstPtr = _To_address(_OFirst);
+        const auto _DFirstPtr = _To_address(_DFirst);
+        const auto _DLastPtr  = _To_address(_DLast);
+        const auto _IFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_IFirstPtr));
+        const auto _OFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_OFirstPtr));
+        const auto _DFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_DFirstPtr));
+        const auto _DLast_ch  = const_cast<char*>(reinterpret_cast<const volatile char*>(_DLastPtr));
+        const auto _Count     = static_cast<size_t>(_DLast_ch - _DFirst_ch);
+        _CSTD memcpy(_OFirst_ch, _IFirst_ch, _Count);
+        if constexpr (is_pointer_v<_InIt>) {
+            _IFirst = reinterpret_cast<_InIt>(_IFirst_ch + _Count);
+        } else {
+            _IFirst += _Count / sizeof(iter_value_t<_InIt>);
+        }
+
+        if constexpr (is_pointer_v<_OutIt>) {
+            _OFirst = reinterpret_cast<_OutIt>(_OFirst_ch + _Count);
+        } else {
+            _OFirst += _Count / sizeof(iter_value_t<_OutIt>);
+        }
+        return {_STD move(_IFirst), _STD move(_OFirst)};
+    }


I believe that overload is superfluous. The only difference is how we get to _Count

I guess we can get to that easier that duplicating all that machinery

We might just use the _count version but that might have worse codegen. #1433 (comment)

I thought more about determining _Count via the right method and passing that around. That should give the optimal codegen all the time

FWIW, I'm happy with this as-is. I think extracting the "distance" behavior from this function would either produce less efficient code or something even more confusing.

CaseyCarter · 2021-05-21T00:01:38Z

+    template <class _InIt, class _OutIt, typename _DistIt>
+    in_out_result<_InIt, _OutIt> _Copy_memcpy_distance(
+        _InIt _IFirst, _OutIt _OFirst, _DistIt _DFirst, _DistIt _DLast) noexcept {
+        const auto _IFirstPtr = _To_address(_IFirst);
+        const auto _OFirstPtr = _To_address(_OFirst);
+        const auto _DFirstPtr = _To_address(_DFirst);
+        const auto _DLastPtr  = _To_address(_DLast);
+        const auto _IFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_IFirstPtr));
+        const auto _OFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_OFirstPtr));
+        const auto _DFirst_ch = const_cast<char*>(reinterpret_cast<const volatile char*>(_DFirstPtr));
+        const auto _DLast_ch  = const_cast<char*>(reinterpret_cast<const volatile char*>(_DLastPtr));
+        const auto _Count     = static_cast<size_t>(_DLast_ch - _DFirst_ch);
+        _CSTD memcpy(_OFirst_ch, _IFirst_ch, _Count);
+        if constexpr (is_pointer_v<_InIt>) {
+            _IFirst = reinterpret_cast<_InIt>(_IFirst_ch + _Count);
+        } else {
+            _IFirst += _Count / sizeof(iter_value_t<_InIt>);
+        }
+
+        if constexpr (is_pointer_v<_OutIt>) {
+            _OFirst = reinterpret_cast<_OutIt>(_OFirst_ch + _Count);
+        } else {
+            _OFirst += _Count / sizeof(iter_value_t<_OutIt>);
+        }
+        return {_STD move(_IFirst), _STD move(_OFirst)};
+    }


FWIW, I'm happy with this as-is. I think extracting the "distance" behavior from this function would either produce less efficient code or something even more confusing.

Co-authored-by: Casey Carter <cartec69@gmail.com>

…l-optimizations

StephanTLavavej

Looks good - I'll validate and push changes for the very minor things I noticed.

StephanTLavavej · 2021-06-10T02:20:06Z

@AdamBucior @CaseyCarter FYI I added _STD move occurrences and verified that the tests are passing, please hiss if I damaged something (I'm 99.999% sure they're good).

StephanTLavavej · 2021-06-10T02:41:38Z

Also had to merge main and fix a stealth merge conflict - now that views are guarded by /std:c++latest after GH-1929, the tests being modified to use views::take_while need to use the concepts_latest_matrix.

StephanTLavavej · 2021-06-12T02:36:35Z

Thanks for increasing the STL's performance to previously unreachable levels! 😹 🚀 📈

Optimizations for unreachable sentinels

4549d9d

AdamBucior requested a review from a team as a code owner April 6, 2021 15:57

clang-format

d20d548

StephanTLavavej added performance Must go faster ranges C++20/23 ranges labels Apr 6, 2021

miscco reviewed Apr 7, 2021

View reviewed changes

AdamBucior added 2 commits April 7, 2021 09:56

Code review

a161a9a

clang-format

fbb5cc2

CaseyCarter self-assigned this Apr 28, 2021

CaseyCarter suggested changes May 21, 2021

View reviewed changes

AdamBucior and others added 7 commits May 21, 2021 08:10

Apply suggestions from code review

1de209e

Co-authored-by: Casey Carter <cartec69@gmail.com>

Code review

5715090

Merge remote-tracking branch 'upstream/main' into unreachable-sentine…

613955b

…l-optimizations

const

79750b2

clang-format

46bbdc9

remove warning suppressions

6f39c5b

workaround clang-format

9791789

CaseyCarter removed their assignment Jun 2, 2021

CaseyCarter approved these changes Jun 9, 2021

View reviewed changes

StephanTLavavej reviewed Jun 10, 2021

View reviewed changes

Comment thread stl/inc/memory Outdated

Comment thread stl/inc/xmemory Outdated

Code review feedback.

8a720b0

StephanTLavavej approved these changes Jun 10, 2021

View reviewed changes

StephanTLavavej added 2 commits June 9, 2021 19:33

Merge branch 'main' into gh1810

9e872c0

Need concepts_latest_matrix.lst for views::take_while.

9d840b6

StephanTLavavej approved these changes Jun 10, 2021

View reviewed changes

CaseyCarter approved these changes Jun 10, 2021

View reviewed changes

StephanTLavavej self-assigned this Jun 11, 2021

StephanTLavavej merged commit e326ec1 into microsoft:main Jun 12, 2021

AdamBucior deleted the unreachable-sentinel-optimizations branch June 12, 2021 09:30

AdamBucior mentioned this pull request Jun 12, 2021

Implement ranges::starts_with and ranges::ends_with #1997

Merged

Conversation

AdamBucior commented Apr 6, 2021 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miscco Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

AdamBucior Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

AdamBucior Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

miscco Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

CaseyCarter May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej commented Jun 10, 2021

Uh oh!

StephanTLavavej commented Jun 10, 2021

Uh oh!

StephanTLavavej commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AdamBucior commented Apr 6, 2021 •

edited by StephanTLavavej

Loading