Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

<type_traits>: is_function simplification#460

Merged
StephanTLavavej merged 15 commits intomicrosoft:masterfrom
ArtemSarmini:is-func-opt
Aug 1, 2020
Merged

<type_traits>: is_function simplification#460
StephanTLavavej merged 15 commits intomicrosoft:masterfrom
ArtemSarmini:is-func-opt

Conversation

@ArtemSarmini
Copy link
Contributor

@ArtemSarminiArtemSarmini commentedJan 25, 2020
edited
Loading

Description

Closes#198. Implementation relies onis_reference andis_const, sois_function was moved down right afteris_volatile (I didn't want to separateis_const andis_volatile).

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), youmust mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@ArtemSarminiArtemSarmini requested a review froma team as acode ownerJanuary 25, 2020 15:53
Copy link
Contributor

@CaseyCarterCaseyCarter left a comment

Choose a reason for hiding this comment

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

Did you intend to update the llvm-project submodule reference? (I suspect not.)

@ArtemSarmini
Copy link
ContributorAuthor

I didn't, so llvm is reverted to original commit

CaseyCarter reacted with thumbs up emoji

@ArtemSarmini
Copy link
ContributorAuthor

ArtemSarmini commentedJan 25, 2020
edited
Loading

Last commit added same optimization tois_member_function_pointer andis_member_object_pointer. I had to move stuff around the file a lot, which is bad, as I heard we tend to have the same declaration order as standard. Maybe _Implementation_details should be defined first in another header like xtraits?

@StephanTLavavej
Copy link
Member

I had to move stuff around the file a lot, which is bad, as I heard we tend to have the same declaration order as standard.

I would say that the general rule is “follow the Standard’s order unless there’s a reason to do otherwise”. Type traits extensively use one another, so their order often varies from what’s depicted in the Standard, and that’s fine.

@ArtemSarmini
Copy link
ContributorAuthor

is_object_v usesis_function_v, so it was moved down.

@BillyONeal
Copy link
Member

Given that this is being billed as a throughput improvement can we get some form of benchmark demonstrating that this change is worth it?

@ArtemSarmini
Copy link
ContributorAuthor

Will try to make one

Copy link
Contributor

@CaseyCarterCaseyCarter left a comment

Choose a reason for hiding this comment

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

I've pushed a commit with a couple of formatting changes that were easier to make than comment. This otherwise seems ready to me.

@StephanTLavavejStephanTLavavej added the throughputMust compile faster labelFeb 5, 2020
@StephanTLavavej
Copy link
Member

Regarding benchmarking, the undocumented compiler option/d1templateStats may provide sufficient evidence. Here's an example:

C:\Temp>type meow.cpp#include <algorithm>#include <stdio.h>#include <vector>using namespace std;int main() {    vector<int> v{1, 7, 2, 9};    sort(v.begin(), v.end());    for (const auto& e : v) {        printf("%d, ", e);    }    printf("\n");}C:\Temp>cl /EHsc /nologo /W4 /MT /d1templateStats meow.cppmeow.cppTotal templates                              : 371Class templates                              : 255Class template specializations               : 56Class template partial specializations       : 200Class template explicit specializations      : 55Class template tentative specializations     : 573Total class template specializations         : 884Most class template specializations          : "__vcrt_va_list_is_reference" (5)Most class template partial specializations  : "std::_Is_function" (36)Most class template explicit specializations : "std::numeric_limits" (18)Most class template tentative specializations: "std::integral_constant" (49)Function templates                           : 49Function template specializations            : 61Function template explicit specializations   : 1Total function template specializations      : 62Most function specializations                : "std::declval" (6)Most function explicit specializations       : "std::_Convert_size" (1)Alias templates                              : 43Alias template specializations               : 242Most alias template specializations          : "std::void_t" (43)Variable templates                           : 24Variable template specializations            : 49Variable template partial specializations    : 4Variable template explicit specializations   : 0Variable template tentative specializations  : 0Most variable template specializations       : "std::is_same_v" (9)Reparsed function template specializations   : 61Substituted function template specializations   : 0

@StephanTLavavej
Copy link
Member

/d1reportTimeSummary can also be used to benchmark this change.

@StephanTLavavejStephanTLavavej added the info neededWe need more info before working on this labelMar 18, 2020
Copy link
Contributor

@barcharcrazbarcharcraz left a comment

Choose a reason for hiding this comment

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

This looks fine to me, I did add comments about some comments (please feel free to comment about my comment comments), but given that they were probably per-existing, and also given that I don't actually care that much about them it's fine if you choose to ignore my suggestions.

removed pointless comments, provided better _Arg_types description
@barcharcraz
Copy link
Contributor

I benched this with clang-cl and cl, As expected I saw very little change on CL.

The testing program:

#include<algorithm>#include<stdio.h>#include<vector>usingnamespacestd;intmain() {    vector<int> v {1,7,2,9};sort(begin(v),end(v));}

msvc before total: 0.110538s
msvc after total: 0.110835s

no change.

However with clang-cl I saw more change
clang-cl before: 451.931 ms
clang-cl after: 187.441 ms

I didn't rebase on master before testing the "after" changes, and I turned off warnings on the stl build (the compile options on the test program were identical.

@BillyONeal
Copy link
Member

Thanks@barcharcraz :)

@StephanTLavavejStephanTLavavej removed the info neededWe need more info before working on this labelJul 8, 2020
* `is_member_function_pointer_v` should ignore cv-qualifiers (easy to fix) and calling conventions (super hard to fix). Bring back the inefficient `_Is_memfunptr` which is not so bad since (1) cl actually implements `is_member_function_pointer_v` in the compiler and (2) we can use an intrinsic for clang.Drive-by: Make `_Weak_types` an alias template.
Copy link
Member

@StephanTLavavejStephanTLavavej left a comment

Choose a reason for hiding this comment

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

There are several test failures, e.g.

"C:\agent\_work\1\s\tests\std\tests\Dev11_0535636_functional_overhaul\test.cpp",line 1219: error: static assertion failed with "TestRWTypes<Pmf0, float, X*, None, None>::value"STATIC_ASSERT(TestRWTypes<Pmf0, float, X*, None, None>::value);

* `_Is_memfunptr` is sensitive to cv-qualifiers, they need to be stripped from its argument* `is_member_pointer_v` needs to work with calling conventions just like `is_member_function_pointer_v` does.
@StephanTLavavej
Copy link
Member

@BillyONeal is concerned that the benchmark might not have been exercisingis_function at all, and that executable startup times may have been producing misleading results. (sort doesn't; it just invokes whatever function object it's been given.)

Copy link
Member

@StephanTLavavejStephanTLavavej left a comment
edited
Loading

Choose a reason for hiding this comment

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

Thanks! I apologize for my extreme delay in reviewing. I've looked through everything and it appears to be correct and simple, which is great. I noticed that the simplification could be extended tois_object_v; I'll validate this and then push a change.

@StephanTLavavejStephanTLavavej merged commit0381c25 intomicrosoft:masterAug 1, 2020
@StephanTLavavej
Copy link
Member

Thanks again for this simplification and compiler throughput improvement, and congratulations on your first microsoft/STL commit! This will ship in VS 2019 16.8 Preview 3. 🎉 😸

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

Reviewers

@StephanTLavavejStephanTLavavejStephanTLavavej approved these changes

@barcharcrazbarcharcrazAwaiting requested review from barcharcraz

+2 more reviewers

@CaseyCarterCaseyCarterCaseyCarter approved these changes

@BillyONealBillyONealBillyONeal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavejStephanTLavavej

Labels

throughputMust compile faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<type_traits>: Consider simplifying is_function

5 participants

@ArtemSarmini@StephanTLavavej@BillyONeal@barcharcraz@CaseyCarter

[8]ページ先頭

©2009-2026 Movatter.jp