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

implement strtotime() via timelib to make it identical to PHP strtotime() #86

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Dec 25, 2020

PHP uses timelib more intensively and we could replace some of out
lookup tables from datetime.cpp with its help, but it might be a better
idea to do this (if ever) later.

This patch also includes a fix to date_default_timezone_get(),
so if anyone uses a date_default_timezone_set() with a supported
argument, calling date_default_timezone_get() would return that value.

Timelib: https://github.com/derickr/timelib
For this WIP I used this fork (cmake is added): https://github.com/quasilyte/timelib/tree/kphp
The fork has 0 changes except for the added CMake file.

TODO:

  • Create a timelib-kphp-vk package for it (like curl-kphp-vk)
  • Move that fork to vkcom organization
  • Can we do a library lookup better instead of using a hardcoded paths?
  • Update the "build from sources" docs
  • Make sure it works on macos

Benchmark results:

name             old time/op  new time/op   delta
Now              1.62µs ± 1%   5.33µs ± 0%  +229.20%  (p=0.016 n=5+4)
Timestamp        2.20µs ± 1%  20.37µs ± 0%  +825.27%  (p=0.008 n=5+5)
10September2000  2.10µs ± 1%   6.86µs ± 1%  +225.76%  (p=0.008 n=5+5)
NextMonday       1.83µs ± 1%   7.04µs ± 1%  +284.92%  (p=0.008 n=5+5)
TimeWithOffset   7.32µs ± 1%   8.59µs ± 0%   +17.41%  (p=0.008 n=5+5)
[Geo mean]       2.51µs        8.53µs       +239.11%

Benchmarks:

<?php

class TimelibBenchmark {
    public function benchmarkNow() {
        return strtotime("now");
    }

    public function benchmarkTimestamp() {
        return strtotime("91248374");
    }

    public function benchmark10September2000() {
        return strtotime("10 September 2000");
    }

    public function benchmarkNextMonday() {
        return strtotime("last Monday");
    }

    public function benchmarkTimeWithOffset() {
        return strtotime("+1 week 2 days 4 hours");
    }
}

@quasilyte quasilyte force-pushed the quasilyte/php_timelib branch 2 times, most recently from 057ed04 to 6069b2b Compare February 16, 2021 11:44
@AlexK0 AlexK0 added the enhancement New feature or request label Feb 18, 2021
@quasilyte quasilyte force-pushed the quasilyte/php_timelib branch 12 times, most recently from f0e0aee to 3b54960 Compare February 26, 2021 13:24
@quasilyte quasilyte changed the title WIP: implement strtotime() via timelib to make it identical to PHP strtotime() implement strtotime() via timelib to make it identical to PHP strtotime() Feb 26, 2021
@quasilyte quasilyte force-pushed the quasilyte/php_timelib branch 3 times, most recently from 6a8fb81 to 8a87021 Compare March 2, 2021 13:46
runtime/datetime.cpp Outdated Show resolved Hide resolved
runtime/timelib_wrapper.cpp Show resolved Hide resolved
runtime/datetime.cpp Outdated Show resolved Hide resolved
runtime/timelib_wrapper.h Outdated Show resolved Hide resolved
runtime/timelib_wrapper.cpp Show resolved Hide resolved
auto now_deleter = vk::finally([now]() { timelib_time_dtor(now); });

// timelib destructors do timelib_free which is a macro that expands to free();
// so we probably don't need a CriticalSectionGuard here
Copy link
Contributor

Choose a reason for hiding this comment

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

timelib_free call is not the main issue. The main issue is an invalid internal library state. If you sure, that timelib doesn't use any static/global vars inside (which can be spoiled with signal interruption), then we don't need a CriticalSectionGuard here, otherwise we must use the guard.

And if you add the guard, the second question is: Is timelib ready for nullptrs from malloc family calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment.

  • timelib can't handle nullptr from malloc gracefully
  • timelib is thread-safe

So it looks like CriticalSectionGuard is not needed here.

@quasilyte quasilyte changed the title implement strtotime() via timelib to make it identical to PHP strtotime() WIP: implement strtotime() via timelib to make it identical to PHP strtotime() Mar 12, 2021
@quasilyte
Copy link
Contributor Author

I addressed the review comments, but the CI build failed.
I'll remove the WIP prefix when it will be clear that failure was unrelated (or fixed).

runtime/datetime.cpp Outdated Show resolved Hide resolved
runtime/datetime.cpp Outdated Show resolved Hide resolved
@quasilyte quasilyte force-pushed the quasilyte/php_timelib branch 2 times, most recently from 246a004 to d1dad72 Compare March 30, 2021 12:48
runtime/datetime.cpp Outdated Show resolved Hide resolved
…me()

PHP uses timelib more intensively and we could replace some of out
lookup tables from datetime.cpp with its help, but it might be a better
idea to do this (if ever) later.

This patch also includes a fix to date_default_timezone_get(),
so if anyone uses a date_default_timezone_set() with a supported
argument, calling date_default_timezone_get() would return that value.
@quasilyte quasilyte changed the title WIP: implement strtotime() via timelib to make it identical to PHP strtotime() implement strtotime() via timelib to make it identical to PHP strtotime() Mar 31, 2021
@quasilyte quasilyte added this to the next milestone Mar 31, 2021
@unserialize unserialize modified the milestone: next Apr 1, 2021
@quasilyte quasilyte merged commit e7be3c3 into master Apr 1, 2021
@quasilyte quasilyte deleted the quasilyte/php_timelib branch April 1, 2021 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants