<chrono>: Cache QueryPerformanceFrequency() and divide just once

[AlexGuteniev]

Description

More aggressive version of PR #646

• One division instead of two
• To avoid overflow, subtract first observed counter from every counter
• To avoid compatibility breaking, add adjusted first counter back to final result
• Caches all in header, so no ABI break, also save on CRT DLL call
• Ad-hoc std::atomic, but previous concerns addressed, now does exactly as real std::atomic

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.

[AlexGuteniev]

I discovered the optimization is incorrect. Original code:

const long long _Part  = (_Ctr % _Freq) * period::den / _Freq;

assumes that _Freq*period::den fits 64 bit.

after change the code assumes is that min(_Freq, period::den)*_Ctr fits 64 bit, which is what the original approach protects against.

[AlexGuteniev]

Now I applied different approach, this may work

[StephanTLavavej]

This is paying a significant amount of implementation complexity - I think we need a strong performance justification for making this change. Can you provide performance numbers for steady_clock::now:

1. Before <chrono>: Cache QueryPerformanceFrequency() #646
2. After <chrono>: Cache QueryPerformanceFrequency() #646, but before this PR
3. After this PR

[AlexGuteniev]

@StephanTLavavej ,
See https://github.com/AlexGuteniev/benchmark_653

My results are
x86:

6042.98  before 646
5904.46  after 646
4520.97  after 653

x64:

3769.84  before 646
3649.27  after 646
2756.91  after 653

It would be simpler if it would go as vNext issue, then no fake atomic needed, it would be possible to move the most of it to .cpp.

[AlexGuteniev]

Another option to achieve the same is to use _mul128 / _div128.
But then the optimization will be applied to x64 only, and it will take bringing in _mul128 / _div128 intrinsics, not currently available in "intrin0.h"

[StephanTLavavej]

FYI @cbezault, I pushed relatively small changes after you approved.

[miscco]

Is it strange if that makes me happy?

[StephanTLavavej]

Thanks for this significant performance improvement! I would have never noticed that this was possible, so I'm glad you did. 😺

[jpark37]

I see this has been backed out since, but is there a reason to use atomics and cache to function-local static variables? Wouldn't it be simpler and faster (no branch) to pull those variables out of the function and initialize them before main()? I see _Query_perf_frequency() in xtime.cpp is also implemented with function static atomic.

This seems better to me, but maybe I'm missing something:

const long long freq_cached = [] {
    LARGE_INTEGER frequency;
    QueryPerformanceFrequency(&frequency);
    return frequency.QuadPart;
}();

long long _Query_perf_frequency() {
    return freq_cached;
}

[BillyONeal]

@jpark That would add a dynamic initializer to the STL and I believe we don't have any of those right now. I seem to recall adding any causing issues with /clr:pure code that tried to statically link the STL in a partially trusted context, since they can't necessarily run any native code here.

But my memory is a bit foggy on the specifics. I know we had to avoid the dynamic initializer but I don't remember the exact technical limitations forcing it.

[BillyONeal]

(Also I don't know if QueryPerformanceFrequency is in the "safe to be called from DllMain" subset since the default answer for "is this safe to call from DllMain" is "no".)

[jpark37]

Thanks for the info.

Aren't people allowed to use STL before main() in general? I've seen people declare STL collections as global variables at least. Right now, if someone calls steady_clock::now() in a global variable initializer, and it's the first call into _Query_perf_frequency(), that will call QueryPerformanceFrequency().

Thinking about it more, it may be unsafe to call my version of _Query_perf_frequency() from another initializer before main() because of not being able to control the order of initialization across translation units, so freq_cached might still be 0 with unlucky order. I don't know if the story is any different with /MT vs. /MD.

[BillyONeal]

Aren't people allowed to use STL before main() in general?

The STL allows people to call it before main(), but if the user is a DLL they still need to observe all the usual platform requirements regarding DllMain.

Right now, if someone calls steady_clock::now() in a global variable initializer, and it's the first call into _Query_perf_frequency(), that will call QueryPerformanceFrequency().

Right, that's a situation we'd like to avoid because it introduces initialization order issues; and QPC isn't explicitly documented as being safe in DllMain.

Thinking about it more, it may be unsafe to call my version of _Query_perf_frequency() from another initializer before main()

It looks correct to me. It might result in 2 threads which both do QPF/QPC but they should still get correct answers.