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

ADD windows support for USE_SYSTEM_TZ_DB #611

Closed
wants to merge 1 commit into from

Conversation

wabscale
Copy link

Windows support for USE_SYSTEM_TZ_DB

These changes add support for the USE_SYSTEM_TZ_DB option on windows. These changes increase the speed of timezone conversion operations by many orders of magnitude (see benchmarks below).

The majority of the changes are in the init_tzdb function. There I added support for the windows file API to load the timezone database off of disk.

This Addresses...

Configuration

Along with these changes, I've added the option to specify the location of the zoneinfo TZDIR by environment variable. To my knowledge, there is no one standard place for the system tz database to live on windows. This means that for those to choose to use USE_SYSTEM_TZ_DB on windows will need to compile their own tz database from iana and specify its location via the TZDIR environment variable. It is my belief that this trade off is well worth it. The performance benefits speak for themselves.

Benchmarks

To test the benefits of the changes I ran this basic benchmark that times many timezone conversions.

see this repo for full implementation

TEST( date, bench1 )
{
    using namespace std::chrono;
    using namespace date;

    int n = 10000;

    auto sw = StopWatch();

    // lookup for every call in tz db,,,
    {
        auto tpUTC = std::chrono::system_clock::now();
        auto const* tz = date::locate_zone( "America/New_York" ); // locate the timezone once

        fmt::print( "locate_zone: time used {} us\n", sw.elapsedUs() );
        sw.restart();
        for( int i = 0; i != n; ++i )
        {
            auto const zt  = make_zoned( tz, tpUTC );
            auto const tp  = zt.get_local_time(); // expensive call
            auto const dp  = floor<days>( tp );
            auto const ymd = year_month_day( dp );
        }
    }
    fmt::print( "lookup: every call: time used {} us\n", sw.elapsedUs() );
    sw.restart();

    // should be way faster
    {
        auto tpUTC = std::chrono::system_clock::now();

        for( int i = 0; i != n; ++i )
        {
            const std::string zone = "America/New_York";
            auto const zt  = make_zoned( zone, tpUTC ); // locate the timezone every time
            auto const tp  = zt.get_local_time();
            auto const dp  = floor<days>( tp );
            auto const ymd = year_month_day( dp );
        }
    }
    fmt::print( "lookup: once      : time used {} us\n", sw.elapsedUs() );
}

Using the regular (non system) timezone database on windows, I get these results:

USE_SYSTEM_TZ_DB=OFF

[ RUN      ] date.bench1
locate_zone: time used 2125059 us
lookup: every call: time used 2041528 us
lookup: once      : time used 2066843 us
[       OK ] date.bench1 (6234 ms)

With these changes, and a compiled iana database:

USE_SYSTEM_TZ_DB=ON

[ RUN      ] date.bench1
locate_zone: time used 43170 us
lookup: every call: time used 52907 us
lookup: once      : time used 803878 us
[       OK ] date.bench1 (900 ms)

Overall these changes allow for about ~7x faster timezone conversions on Windows.

Comment on lines +3715 to +3725
return get_tzdb().locate_zone(
#if WIN32 && USE_OS_TZDB
// When USE_OS_TZDB=ON, the tz_name needs to be a valid path.
// For timezone labeling consistency in Windows, switch "/" to "\".
// This will allow for tz_name to have consistent naming regardless of
// if USE_OS_TZDB is ON.
std::regex_replace(tz_name, std::regex("\\/"), "\\")
#else
tz_name
#endif
);
Copy link
Contributor

@Tachi107 Tachi107 Aug 4, 2022

Choose a reason for hiding this comment

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

Instead of using std::regex, why not std::replace? Here you just need to replace a single character.

Edit: oh, tz_name is a const&, would it be possible to change it to an std::string passed by value? You'd have to create a copy of the string in either case, but at least with replace you don't need to use regex (that is broken on some old compilers)

Suggested change
return get_tzdb().locate_zone(
#if WIN32 && USE_OS_TZDB
// When USE_OS_TZDB=ON, the tz_name needs to be a valid path.
// For timezone labeling consistency in Windows, switch "/" to "\".
// This will allow for tz_name to have consistent naming regardless of
// if USE_OS_TZDB is ON.
std::regex_replace(tz_name, std::regex("\\/"), "\\")
#else
tz_name
#endif
);
#if WIN32 && USE_OS_TZDB
// When USE_OS_TZDB=ON, the tz_name needs to be a valid path.
// For timezone labeling consistency in Windows, switch "/" to "\".
// This will allow for tz_name to have consistent naming regardless of
// if USE_OS_TZDB is ON.
std::replace(tz_name.begin(), tz_name.end(), '/', '\\');
#endif
return get_tzdb().locate_zone(tz_name);

@samuel-emrys
Copy link

I'm interested in these changes from a packaging perspective - this is pretty stale though, is there likely to be any movement on merging this in?

@HowardHinnant
Copy link
Owner

For Windows platforms I strongly encourage people to migrate to C++20 <chrono>.

@trahuel
Copy link

trahuel commented Aug 18, 2023

According to Microsoft documentation, time zone support is only available on

  • Windows 10 version 1903/19H1 or later
  • Windows Server 2022 or later

If my project is compiled with C++20, i will not be able to use it on Windows Server 2019, am i right ?
In that case, i still need to use your library ?

@samuel-emrys
Copy link

@wabscale can you comment on the utility of the TZDATA_DIR vs TZDIR environment variables? why are these not the same in this PR?

@wabscale
Copy link
Author

@wabscale can you comment on the utility of the TZDATA_DIR vs TZDIR environment variables? why are these not the same in this PR?

That appears to be a mistake on my part. If this ever makes it into this library, this PR will need to be rewritten anyway. My original repo is many commits behind because of how long this issue has been open.

samuel-emrys added a commit to samuel-emrys/conan-center-index that referenced this pull request Dec 21, 2023
* Add patches for v3.0.0 and v2.4.1 that hotfix the changes present in
  HowardHinnant/date#611 on to these older versions. This will allow all
  versions of date currently on CCI to utilise the tz package as a data
  source for the timezone database.

  Some additional modification of the build system was required in these
  patches to ensure a consistent interface across versions. Only the
  minimal changes necessary to introduce desired features were made,
  preserving deficiencies associated with older versions of date.
samuel-emrys added a commit to samuel-emrys/conan-center-index that referenced this pull request Dec 21, 2023
* Add patches for v3.0.0 and v2.4.1 that hotfix the changes present in
  HowardHinnant/date#611 on to these older versions. This will allow all
  versions of date currently on CCI to utilise the tz package as a data
  source for the timezone database.

  Some additional modification of the build system was required in these
  patches to ensure a consistent interface across versions. Only the
  minimal changes necessary to introduce desired features were made,
  preserving deficiencies associated with older versions of date.
samuel-emrys added a commit to samuel-emrys/conan-center-index that referenced this pull request Jan 13, 2024
* Add tz recipe as a dependency to provide a way of easily
  consuming/maintaining and upgrading the timezone database
* Apply patch from HowardHinnant/date#807 to add the ability to specify
  the tz database by environment variable
* Deprecate `use_system_tz_db` in favour of `with_tzdb` option to handle
  all mutually exclusive options
* Add `with_db_format` option to provide flexibility in how the tz
  package is consumed.

This implementation superesedes conan-io#16285 and conan-io#21719. In the case of conan-io#21719,
it turned out that HowardHinnant/date#611 was not mature enough to
provide the ability for the date library to read zic-compiled binary
variants of the tzdb on Windows. To take an iterative approach, this PR
aims to extract the elements that worked from conan-io#21719, which was the use
of an environment variable to inject the conan packaged tz database.

Use of a binary database has been marked as an invalid configuration on
Windows. This functionality can be added at a later date.

This pull request is blocked on conan-io#21671, which packages the source
database in the tz recipe and makes this the default configuration since
this is the only database that can be parsed consistently on all major
platforms.

Closes conan-io#16204
@SylvainCorlay
Copy link

We have been using these changes to package this project for conda-forge - and I can confirm that it performs as expected.

One way to make the patch super minimal is to simply use dirent on windows as a build-time dependency. If this can help getting this proposal in?

After all, the resulting changes would not be very windows specific:

  • enabling specifying the tzdata installation directory through the standard TZDIR environment variable.
  • specifying folder_delimiter instead of '/' in a couple of places.

Would you consider such a change @HowardHinnant ?

@basilgello
Copy link
Contributor

As someone who succeeded using native C++20 <chrono> features for MSVC 2019+ and newer GCC/Clang compilers in xbmc/xbmc#18727 in combination with date 3.0.1 for other platforms, I dont think this PR has any benefits in comparison to Howard's recommended approach. Commenting here to let others use my implementation as source of inspiration :)

@senstar-nross
Copy link

As someone who succeeded using native C++20 <chrono> features for MSVC 2019+ and newer GCC/Clang compilers in xbmc/xbmc#18727 in combination with date 3.0.1 for other platforms, I dont think this PR has any benefits in comparison to Howard's recommended approach. Commenting here to let others use my implementation as source of inspiration :)

This is really a shame that this has been closed unimplemented. It seems like a small change that would benefit those of use that have to support older OS versions.

Just because std::chrono works now doesn't mean this isn't useful. Comments like "look what i did to port to c++20 so this isn't necessary to anyone else" are unhelpful ... In my particular case I am using c++20 and now need to fix a crash on older OS's

and was actually led here from this microsoft/STL#1911 where date is a recommended solution.

as a preview poster said

According to Microsoft documentation, time zone support is only available on
Windows 10 version 1903/19H1 or later
Windows Server 2022 or later

Windows 10 Iot/Enterprise, Server 2019 and other LTSB releases are "supported" by microsoft til 2029.

Anyway I am mostly just venting because much of the development world is so quick to say, "oh just update to the latest" and really ignores the fact that a very large segment of software that still needs support and is under current development is running on older hardware with older OSes.

I am glad date is here, and this PR maybe something i can use...
so i will either fork and spend the time to make it use the system database
or deal with tz databases/installers etc.

(I was just so disappointed to get error C1189: #error: "USE_OS_TZDB can not be used on Windows")

@basilgello
Copy link
Contributor

No rush - I had to work around the buggy STL implementation so I made https://github.com/basilgello/date/tree/tz-windows but in case something older than Windows 10 1703 is to be used, there is even another one implementation in progress, using "just" WinAPI.

@senstar-nross
Copy link

@basilgello hey thanks! I tried it out and got it to work on windows 11 (there were some types in tz.cpp that were included from stl/src/tzdb.cpp that were conflicting (i didn't investigate why i just commented them out to get it to compile).

But when I tried it on 1809 I found it still tries to load icu.dll (which is not installed until 1903)...

The code appears to attempt to use the icuin and icuuc dlls to get the functions but maybe that code is unfinished/untested? I could make a PR with the changes if you wanted to see them. (it didn't take very long to fix)

@basilgello
Copy link
Contributor

For 1703 to 1903 the icuin/icuuc.dll must be loaded. I had no chance to test that codepath, so if you confirm it tries to load wrong dll, I can fix it.

@basilgello
Copy link
Contributor

I have not PR'd the Windows tz support yet but I am glad to add your commit into my branch to preserve attribution! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants