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

Run only one of mini_mktime(), mktime() #21885

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

khwilliamson
Copy link
Contributor

@khwilliamson khwilliamson commented Jan 25, 2024

mini_mktime() is a faster, stripped down version of mktime() that ignores locale-dependent information. But it doesn't work when the structure it fills in contains fields that are locale-related. A patch 20 some years ago called mktime() to fill in the missing fields.

But that ends up executing similar algorithms twice. If we need the locale-related info, why not just call mktime() once and be done with it. I figured I must be missing something, so started a smoke-me branch with this change. https://perl.develop-help.com/?b=smoke-me%2Fkhw-mktime. So far everything is passing.

There is one glitch on my Linux glibc. mktime() did not accept 60 for the second number (a leap second) , and mini_mktime() does. And there is a test that includes a leap second.

The right thing to do to calculate the number of seconds since the epoch is to have a table of when actual leap seconds happened, and take that into account. I guess there haven't been enough of them to make a difference. https://en.wikipedia.org/wiki/Leap_second has an extended discussion on this.

This commit uses plain mktime() when the fields exist that are not populated by min_mktime

@Tux
Copy link
Contributor

Tux commented Jan 26, 2024

Nice analysis!

@tonycoz
Copy link
Contributor

tonycoz commented Jan 28, 2024

... this change. https://perl.develop-help.com/?b=smoke-me%2Fkhw-mktime. So far ...

When you use the link creation tool in the github editor it creates a link using the highlighted text as the text of the link, not the url of the link. You need to replace the "url" in the produced markdown to make it a useful link.

eg. from your markdown [https://perl.develop-help.com/?b=smoke-me%2Fkhw-mktime](url), which is something you've done a few times in other issues and pull requests (including the wikipedia link)

If you want the link text like https://perl.develop-help.com/?b=smoke-me%2Fkhw-mktime to just be both the text and url of a link don't bother with the link creation tool, github will linkify it for you and I can stop visiting https://github.com/Perl/perl5/pull/url 🙂

Comment on lines 295 to 297
# This is an actual time a leap second occurred.
# https://en.wikipedia.org/wiki/Leap_second */
try_strftime("Mon Jun 30 23:59:60 1997 181", 60,59,23, 30,5,97);
Copy link
Contributor

Choose a reason for hiding this comment

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

strftime() works in local time, but leap seconds are added at midnight UTC, not localtime.

So this change only matters when the current time zone is UTC (Britain in winter, some parts of Africa I assume)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my analysis was flawed.

Now updated to detect mktime not handling leap seconds like mini_mktime does and to override with the latter

mini_mktime() has less overhead than libc mkttime(), but it doesn't get
all the desired fields on all platforms.  Prior to this commit, both got
called on such platforms, roughly doubling the amount of time spent.
So just use the slightly more expensive function on those platforms and
avoid that redundant overhead.

The only glitch I've seen is that mktime() likely doesn't handle leap
seconds the same way mini_mktime() does; so this calls both functions
for those very rare instances.
@khwilliamson khwilliamson merged commit 36e41d5 into Perl:blead Feb 7, 2024
28 checks passed
@khwilliamson khwilliamson deleted the mktime branch February 7, 2024 17:59
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.

3 participants