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

<chrono>: Performance: consider GetSystemTimeAsFileTime instead of GetSystemTimePreciseAsFileTime #624

Closed
AlexGuteniev opened this issue Mar 19, 2020 · 8 comments
Labels
performance Must go faster wontfix This will not be worked on

Comments

@AlexGuteniev
Copy link
Contributor

Currently std::chrono::system_clock::now() is GetSystemTimePreciseAsFileTime() on Vista+

Consider just using GetSystemTimeAsFileTime(), as it is way faster, as it does not use QueryPerformanceCounter() uderneath.

If user wants resolution, they would call high_resolution_clock, which is steady_clock, and it directly calls QueryPerformanceCounter().

Potentially breaking change, but not ABI-breaking.

@BillyONeal
Copy link
Member

@StephanTLavavej wasn't there an internal version of this bug?

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Mar 23, 2020
@StephanTLavavej
Copy link
Member

Not to my knowledge - I intentionally used Precise as non-Precise is so coarse-grained. I wasn't aware that Precise was slower; my understanding was that QPC was very fast.

Can you quantify what "way faster" is?

@StephanTLavavej StephanTLavavej added performance Must go faster and removed enhancement Something can be improved labels Mar 24, 2020
@StephanTLavavej StephanTLavavej changed the title <chrono>: Performace: consider GetSystemTimeAsFileTime instead of GetSystemTimePreciseAsFileTime <chrono>: Performance: consider GetSystemTimeAsFileTime instead of GetSystemTimePreciseAsFileTime Mar 24, 2020
@BillyONeal
Copy link
Member

@StephanTLavavej Maybe I'm remembering DevCom-184829 which was filed against the UCRT

@StephanTLavavej
Copy link
Member

Oh yeah, the VM interaction. system_clock doesn't have second granularity, so the concern with time() doesn't apply.

@AlexGuteniev
Copy link
Contributor Author

Simple performance test:

#include <chrono>
#include <iostream>
#include <Windows.h>

int main()
{
    using namespace std::chrono;
    constexpr std::size_t N = 1'000'000;

    FILETIME ft;

    auto start_time_point = steady_clock::now();
    for (std::size_t i = 0; i != N; i++) {
        ::GetSystemTimeAsFileTime(&ft);
    }
    auto duration_ms = duration_cast<duration<double, std::milli>>(
        steady_clock::now() - start_time_point);
    std::cout << duration_ms.count() << " ms GetSystemTimeAsFileTime\n";

    start_time_point = std::chrono::steady_clock::now();
    for (std::size_t i = 0; i != N; i++) {
        ::GetSystemTimePreciseAsFileTime(&ft);
    }
    duration_ms = duration_cast<duration<double, std::milli>>(
        steady_clock::now() - start_time_point);
    std::cout << duration_ms.count() << " ms GetSystemTimePreciseAsFileTime\n";
}

Result on my system:

1.8054 ms GetSystemTimeAsFileTime
18.7456 ms GetSystemTimePreciseAsFileTime

I believe that results would vary. It all depends on performance of rdtsc instruction on a given CPU.
QPC is wrapper for rdtsc.

I intentionally used Precise as non-Precise is so coarse-grained

This is also true, so I'm not sure if 10 times performance difference advocates this change.

@AlexGuteniev
Copy link
Contributor Author

Note that the same way steady_clock could be separated from high_resolution_clock and use GetTickCount64.

(It is actually my production case; I needed fast and steady clock. Luckily, implementing Clock based on GetTickCount64 is trivial, as the unit is hard-defined)

@Lectem
Copy link

Lectem commented Mar 25, 2020

Note that the same way steady_clock could be separated from high_resolution_clock and use GetTickCount64.

While it could, I think it would be better to keep using QPC for steady_clock, as high_resolution_clock is kind of shunned now (even Howard Hinnant said it was a mistake to standardize it). As a result, quite a bit of code expects steady_clock to be high resolution.

@StephanTLavavej
Copy link
Member

We talked about this in our weekly meeting, and we believe that making a runtime behavior change here would be destabilizing. We suggest writing a clock powered by GetSystemTimeAsFileTime if you want that function's behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants