Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize std::count even more for vector<👻> #2203

Merged
merged 25 commits into from
Oct 20, 2021

Conversation

AlexGuteniev
Copy link
Contributor

Continuation of #2201

Benchmark (compare run with and without changes):

// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <chrono>
#include <limits>
#include <iostream>
#include <vector>

volatile int result;

int main(int argc, char** argv)
{
    const int N = 20000;
    std::vector<bool> boo(1000000);

    std::chrono::steady_clock::duration d{};
    std::chrono::steady_clock::time_point t;

    t = std::chrono::steady_clock::now();
    for (int i = 0; i < N; ++i) {
        result = std::count(boo.begin(), boo.end(), true);
    }
    d = std::chrono::steady_clock::now() - t;

    std::cout << "t\t" << std::chrono::duration_cast<std::chrono::duration<double>>(d) << "\n";
}

@CaseyCarter CaseyCarter added the performance Must go faster label Sep 14, 2021
stl/inc/vector Outdated Show resolved Hide resolved
@AlexGuteniev AlexGuteniev marked this pull request as ready for review September 15, 2021 19:08
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner September 15, 2021 19:08
@StephanTLavavej
Copy link
Member

Moving back to WIP until #2201 is merged, to avoid any confusion for maintainers. (This is just so we'll be able to review a simpler diff; no concerns with the actual changes at this time.)

@CaseyCarter
Copy link
Member

Also from our weekly meeting: vector<👻> is hilarious and we're keeping the nickname.

Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

These look good, still a bit frustrated the optimizer can't figure this out.

@AlexGuteniev
Copy link
Contributor Author

still a bit frustrated the optimizer can't figure this out

I was able to convey how to reproduce the optimizer bug, from DevCom-1527995 :

We can reproduce your issue. We’ve filed a bug for this issue on the C++ team here.

It is in Under investigation status now. So maybe it will be fixed, and we may avoid some of the tricks.

@barcharcraz
Copy link
Member

yeah, I wish the compiler was better at hoisting the checks out of the loops by itself, oh well.

@barcharcraz barcharcraz removed their assignment Oct 11, 2021
@StephanTLavavej StephanTLavavej self-assigned this Oct 13, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good, I'll push a small comment change!

stl/inc/limits Outdated Show resolved Hide resolved
stl/inc/vector Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Oct 19, 2021
@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR; please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit ce9b088 into microsoft:main Oct 20, 2021
@StephanTLavavej
Copy link
Member

Thanks for this haunted PR! 👻 🦇 😸

@AlexGuteniev AlexGuteniev deleted the vector_boo branch October 20, 2021 10:29
AreaZR pushed a commit to AreaZR/STL that referenced this pull request Nov 4, 2021
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants