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>: Optimize to_sys and to_local #3579

Merged
merged 4 commits into from
Mar 28, 2023

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Mar 17, 2023

... by only retrieving necessary information.

This makes to_local 3x faster on my machine (although it's still hundreds times slower than localtime).

@cpplearner cpplearner requested a review from a team as a code owner March 17, 2023 10:21
@StephanTLavavej StephanTLavavej added performance Must go faster chrono C++20 chrono labels Mar 17, 2023
stl/src/msvcp_atomic_wait.src Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I believe that this is correct and ABI-safe.

In particular, I believe that it doesn't need to wait for the 17.8 Preview 1 unlocked redist. That's because both forms of mixing are compatible:

Code Redist Result
Old Old Status quo, full info, slow but correct
Old New Bincompat scenario, old code's null terminator is interpreted as _Full, slow but correct
New Old This scenario is usually forbidden and will persist until 17.8p1!
New code's "type terminator" is ignored, old redist strictly avoids reading beyond the given length, full info returned, slow but correct
New New Desired state, fast and correct

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Minor comments requested.

stl/inc/__msvc_tzdb.hpp Show resolved Hide resolved
stl/inc/__msvc_tzdb.hpp Outdated Show resolved Hide resolved
@@ -545,6 +555,10 @@ void __stdcall __std_tzdb_delete_current_zone(__std_tzdb_current_zone_info* cons
return _Report_error(_Info, __std_tzdb_error::_Icu_error);
}

if (_Type == __std_tzdb_sys_info_type::_Offset_and_range) {
return _Info.release();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that if _Type \notin {0, 1, 2}, you'll get the full info; this means that future extensions will get all of the info, which seems reasonable, and means it'll be easy to extend. (no change requested, this is great!)

@strega-nil-ms
Copy link
Contributor

@StephanTLavavej pushed changes after you approved.

@StephanTLavavej
Copy link
Member

Thanks, great comment! 😻

@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit d27f041 into microsoft:main Mar 28, 2023
@StephanTLavavej
Copy link
Member

Thanks - must go faster!

🚗 🦖 ⌚

@cpplearner cpplearner deleted the optimize-get-info branch April 1, 2023 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants