<functional> Avoid layers of forwards in invoke #585

BillyONeal · 2020-03-04T08:52:01Z

Description

This change removes the last "nonessential" stack frame that would be encountered for threads, and makes the debugging experience for several standard components, like std::function, nicer by not needing so many step into / step out of sequences.

CRT's thread entry point
std::thread's entry point that staples the parameters on
invoke
already user code yay!

Hopefully this makes debug codegen better too, particularly now that ranges hammers invoke even harder than we used to.

I didn't change any of the metaprogramming for deciding which strategy to use -- I like that we don't use SFINAE to make that decision, and don't really consider myself competent enough a metaprogrammer to confidently make changes there. I just make _Invoker_xyz also supply a strategy value that is fed into if constexpr. @CaseyCarter suggested some larger changes which might have sped up metaprogramming I tried, but ran into issues because one can't deduce the calling convention of pointers-to-member-function.

I've also made a one time exception to our usual policy of using std::forward rather than static_cast, with the rationale that invoke is hammered everywhere, and also by traits, and we want the debugging experience of that to be as nice as possible.

(Also drive-by removed unnecessary compilation of iostreams from the Dev10_729003_bind_reference_wrapper I noticed debugging a test case failure)

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

…s, working around DevCom-936694 and DevCom-936696.

stl/inc/type_traits

CaseyCarter

Some little tweaky things - and thanks for the investigation with my tiny invoke implementation.

stl/inc/type_traits

stl/inc/xatomic.h

stl/inc/type_traits

CaseyCarter · 2020-03-04T16:22:07Z

stl/inc/type_traits

+    return _Invoker1<_Callable, _Ty1>::_Call(
+        static_cast<_Callable&&>(_Obj), static_cast<_Ty1&&>(_Arg1), static_cast<_Types2&&>(_Args2)...);
+#endif // _HAS_IF_CONSTEXPR
+}

 #if _HAS_CXX17


Should we consider #define _C_invoke invoke in C++20 mode to potentially reduce total instantiations when a program calls both invoke(args...) and _C_invoke(args...)? Or:

#if _HAS_CXX20 #define _C_INVOKE invoke #else #define _C_INVOKE _C_invoke // define _C_invoke here #endif

and replace _C_invoke with _C_INVOKE at the callsites?

I'm inclined to think that we should implement #51 unconditionally and just have invoke be constexpr. WG21-P0859 "Core Issue 1581: When are constexpr member functions defined?" was voted in as a Defect Report in Albuquerque, which means that it applies retroactively to C++14 (so we can report any issues as compiler bugs). As for the library side, our general policy is to guard features for Standard modes, unless the feature is non-disruptive and guarding it would increase complexity significantly. So far, our experience with constexpr has been that while it can trigger compiler bugs, it almost always does so when the user is attempting to use it in a constant expression, not ordinary calls (that's why we were able to constexprize so much of the STL without the world exploding). Additionally, making a function constexpr is non-disruptive to people who aren't expecting it - it introduces no new identifiers, doesn't change lookup or throughput, etc. (The Library's prohibition on constexpr strengthening does not make sense anymore.) At most, people might take a dependency on the feature and then notice that other implementations do guard it - but that seems like a minor risk (entirely at compile time).

The throughput cost of having two invoke implementations is nonzero, and the complexity cost (horrible macro) is significant, so I think it should just be unconditionally constexpr.

Ditto "I shouldn't be trying to trick you into implementing #51, nevermind for now."

Reactivating: I'm going to do just the std::invoke itself only part of #51 unconditionally.

Implementing that makes this change depend on #565 since you want to land that out-of-band in 16.6, so I'll hold off merging this until you've done that out of cycle stuff.

s/unconditionally/in C++17/g because our rationale is "apply" and friends added in 17

tests/std/tests/Dev10_729003_bind_reference_wrapper/test.cpp

stl/inc/type_traits

* de-std:: * _C_invoke => _STD invoke * Note that the LWG issue part of P1065R2 is implemented under '17. * Remove decay of _Pmf/_Pmd * PMD noexcept * Remove forwarding of reference_wrapper. * Strengthen noexcept and punch through reference_wrapper.

…tests for the constexpr invoke function.

StephanTLavavej

Looks good, two comment changes requested.

stl/inc/yvals_core.h

StephanTLavavej · 2020-03-05T03:17:08Z

stl/inc/type_traits

+public:
+    template <class... _Types>
+    auto operator()(_Types&&... _Args) const
+        noexcept(noexcept(_STD invoke(*_Ptr, static_cast<_Types&&>(_Args)...))) // Strengthened


We conventionally say strengthened in lowercase.

StephanTLavavej · 2020-03-05T03:19:04Z

tests/std/tests/Dev11_0535636_functional_overhaul/test.cpp

@@ -1032,21 +1060,76 @@ void test_reference_wrapper_invocation() {


 // Test C++17 invoke().
+#if _HAS_CXX17
+constexpr bool test_invoke_constexpr() {
+    // MSVC++ implements LWG-2894 as a DR back to C++17


We usually refer to ourselves as MSVC without ++.

You don't work for "MSVD"?

MSVC++ => msvc.

tests/std/tests/Dev09_158181_tr1_unordered_meow_swap/test.cpp

tests/std/tests/Dev11_0535636_functional_overhaul/test.cpp

tests/std/tests/VSO_0000000_instantiate_containers/test.cpp

Co-Authored-By: Casey Carter <[email protected]>

StephanTLavavej · 2020-04-20T23:59:13Z

This resolved Microsoft-internal VSO-231629 "<functional>: Investigate reducing the number of helper functions instantiated by std::function".

BillyONeal added 6 commits March 3, 2020 18:03

Demacroize invoke.

9390911

Avoid needless <sstream> in Dev10_729003_bind_reference_wrapper.

3471605

if-constexpr-ize invoke so that there is at most one layer of forward…

623fe9a

…s, working around DevCom-936694 and DevCom-936696.

_Invoker => _Invoker1

14f948e

de-forward invoke

c5af089

Workaround DevCom-936696 again.

8f38b2f

BillyONeal requested a review from a team as a code owner March 4, 2020 08:52

BillyONeal self-assigned this Mar 4, 2020

BillyONeal commented Mar 4, 2020

View reviewed changes

stl/inc/type_traits Outdated Show resolved Hide resolved

stl/inc/type_traits Outdated Show resolved Hide resolved

stl/inc/type_traits Outdated Show resolved Hide resolved

stl/inc/type_traits Show resolved Hide resolved

git commit -am "Remove some comments and clang-format."

df0bd58

CaseyCarter suggested changes Mar 4, 2020

View reviewed changes

StephanTLavavej added the enhancement Something can be improved label Mar 4, 2020

StephanTLavavej requested changes Mar 4, 2020

View reviewed changes

BillyONeal added 3 commits March 4, 2020 16:02

PR feedback:

ab1e828

* de-std:: * _C_invoke => _STD invoke * Note that the LWG issue part of P1065R2 is implemented under '17. * Remove decay of _Pmf/_Pmd * PMD noexcept * Remove forwarding of reference_wrapper. * Strengthen noexcept and punch through reference_wrapper.

Fix declaration order problem for _Ptr in reference_wrapper, and add …

47ec84b

…tests for the constexpr invoke function.

Workaround no noexcept in the type system in C++14 and DevCom-939490.

e58e3f7

BillyONeal requested review from StephanTLavavej and CaseyCarter March 5, 2020 03:03

clang-format

ad44e88

StephanTLavavej approved these changes Mar 5, 2020

View reviewed changes

CaseyCarter approved these changes Mar 5, 2020

View reviewed changes

BillyONeal added 3 commits March 5, 2020 01:08

Merge remote-tracking branch 'origin/master' into fast_invoke

0f29c02

More _C_invokes.

45aa3e3

Strengthened => strengthened.

12913ab

MSVC++ => msvc.

BillyONeal requested a review from StephanTLavavej March 5, 2020 09:15

CaseyCarter approved these changes Mar 5, 2020

View reviewed changes

tests/std/tests/Dev09_158181_tr1_unordered_meow_swap/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/Dev11_0535636_functional_overhaul/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/VSO_0000000_instantiate_containers/test.cpp Show resolved Hide resolved

BillyONeal and others added 3 commits March 5, 2020 12:53

Casey CR changes

ff9e9f5

Co-Authored-By: Casey Carter <[email protected]>

One more nitpick.

765672a

clang-format

96b9b44

StephanTLavavej approved these changes Mar 5, 2020

View reviewed changes

BillyONeal merged commit 0d75fc5 into microsoft:master Mar 5, 2020

BillyONeal deleted the fast_invoke branch March 5, 2020 23:36

StephanTLavavej mentioned this pull request Mar 25, 2020

Fix #239: Improve _Invoke_traits throughput #408

Merged

4 tasks

StephanTLavavej added a commit to lozinska/STL that referenced this pull request Mar 26, 2020

Merge with microsoftGH-585.

a660e02

frederick-vs-ja mentioned this pull request Oct 11, 2023

Should we make invoke constexpr in all modes? #4080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<functional> Avoid layers of forwards in invoke #585

<functional> Avoid layers of forwards in invoke #585

BillyONeal commented Mar 4, 2020

CaseyCarter left a comment

CaseyCarter Mar 4, 2020

StephanTLavavej Mar 4, 2020

CaseyCarter Mar 4, 2020

BillyONeal Mar 4, 2020

BillyONeal Mar 4, 2020

BillyONeal Mar 5, 2020

StephanTLavavej left a comment

StephanTLavavej Mar 5, 2020

StephanTLavavej Mar 5, 2020

BillyONeal Mar 5, 2020

StephanTLavavej commented Apr 20, 2020

<functional> Avoid layers of forwards in invoke #585

<functional> Avoid layers of forwards in invoke #585

Conversation

BillyONeal commented Mar 4, 2020

Description

Checklist

CaseyCarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej commented Apr 20, 2020