Skip to content

Conversation

@lozinska
Copy link
Contributor

@lozinska lozinska commented Jan 5, 2020

Description

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.

@lozinska lozinska requested a review from a team as a code owner January 5, 2020 03:49
@BillyONeal
Copy link
Member

Does this make it more difficult to if constexpr-ize invoke by virtue of exposing its implementation details in more places?

@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label Jan 9, 2020
@StephanTLavavej
Copy link
Member

Does this make it more difficult to if constexpr-ize invoke by virtue of exposing its implementation details in more places?

Maybe, but we can always undo or rework this later.

While this change successfully built and passed all of the STL's tests, it failed in the compiler's test suite, with the internal-only "checked" build of the compiler (which contains additional assertions to verify compiler state). The failure looks like:

996233_2.cpp(4): fatal error C1075: '{': no matching token found
D:\Contest\xlgmztty.fyl\binaries\x86chk\inc\type_traits(1733): Expectation failed: EXPECTATION_UNREACHABLE, file d:\agent\_work\2\s\src\vctools\Compiler\CxxFE\sl\p1\c\p0gettok.c, line 11957

This points to _Invoke_traits, so I'm confident that it was caused by this change. As far as I can tell, nothing is wrong with the change itself, it's simply triggering a pre-existing bug in the compiler. I'll need to reduce this to a minimal test case for the compiler team to investigate and fix; in the meantime, this PR is blocked, so I've marked it accordingly. Thanks for your patience.


template <class... _Types>
struct _Invoke_traits<void_t<decltype(_STD invoke(_STD declval<_Types>()...))>,
struct _Invoke_traits<void_t<decltype(_Invoker<_Types...>::_Call(_STD declval<_Types>()...))>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler folk have mentioned in the past that MSVC doesn't memoize the results of overload resolution but does memoize alias template expansions. I suspect we could improve throughput here by adding an alias template for decltype(_Invoker<_Types...>::_Call(_STD declval<_Types>()...)) a la:

template <class... _Types>
using _Meow = decltype(_Invoker<_Types...>::_Call(_STD declval<_Types>()...));

and replacing the two occurrences of decltype(_Invoker<_Types...>::_Call(_STD declval<_Types>()...)) here with _Meow<_Types...>.

@StephanTLavavej
Copy link
Member

Thanks to Xiang Fan, the compiler bug has been fixed by Microsoft-internal MSVC-PR-222035 which will be available in VS 2019 16.6 (presumably Preview 1); we need to wait for that release, as we currently don't have a faster way to consume unreleased compilers.

@StephanTLavavej StephanTLavavej added blocked on VS 2019 16.6 Preview 1 and removed blocked Something is preventing work on this labels Jan 24, 2020
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Feb 5, 2020
@StephanTLavavej StephanTLavavej linked an issue Feb 7, 2020 that may be closed by this pull request
@StephanTLavavej
Copy link
Member

Unfortunately, we merged #585 which requires this PR to be updated.

@StephanTLavavej
Copy link
Member

I'm testing an update.

@StephanTLavavej StephanTLavavej merged commit 6d88a88 into microsoft:master Mar 26, 2020
@StephanTLavavej
Copy link
Member

I went ahead and merged this since it passed our Microsoft-internal build and tests. (We checked in a GitHub test harness today, but we're still working through sporadic failures and long queue times.)

Thanks for the throughput improvement! Compile times are a top customer concern, so every bit helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

throughput Must compile faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<type_traits>: Improve _Invoke_traits throughput

5 participants