Skip to content

Refactor Internal Time Retrieval Handling for Improved Consistency and Resolution #19202

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.

Sign up for GitHub

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

Jump to bottom
Open
marc-mabe wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor Internal Time Retrieval Handling for Improved Consistency and Resolution #19202

marc-mabe wants to merge 1 commit into from
+634 −904

Conversation

@marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Jul 21, 2025 •
edited
Loading

This pull request refactors how PHP internally handles current time retrieval and time measurement, introducing a more consistent and portable approach. The primary goals are to encapsulate platform-specific differences, improve time resolution, and prepare for long-term compatibility (e.g., Y2038 on WIN64).

Key Changes

  • Introduced zend_time.h
    A new internal header that abstracts system time functions, replacing direct usage of <time.h> and related platform-specific APIs.

  • Added zend_time_real_spec
    A unified wrapper around clock_gettime(), timespec_get(), gettimeofday(), and time() using the real/wall clock.
    Returns a timespec structure with nanosecond precision (when available).

  • Added zend_time_real_get
    A lightweight wrapper around time(NULL) for simple, low-resolution time retrieval.

  • Added zend_time_mono_fallback
    A wrapper for zend_hrtime or falls back to zend_time_real_spec.
    Useful for time measurements (e.g. timeout handling) where monotonic time is preferred, but wall time is an acceptable fallback.

  • Added helper functions
    Added utilities to simplify usage of timeval and timespec structures.

  • Replaced direct system time API usage
    Internal calls to system time functions now use zend_time_*.

  • Standardized time representation
    Transitioned to timespec for current time where appropriate, while retaining timeval for files and stream-related functionality.

Benefits

  • Improved Portability and Readability
    Platform-specific time logic is encapsulated, reducing conditionals and improving clarity.

  • Guaranteed Time API Availability
    The new abstraction ensures time can always be retrieved via a fallback chain:
    clock_gettime() → timespec_get() → gettimeofday() → time()

  • Y2038 Compatibility on WIN64
    Addresses issues caused by long in timeval.tv_sec on 64-bit Windows systems.

    Related issue: #17856 – time() and Y2038 problem on 64-bit Windows

  • Improved Resolution for Time Functions
    microtime() and gettimeofday(true) now offer better time precision without breaking backward compatibility.

  • Removed Redundant Availability Checks
    Previously unverified use of gettimeofday() has been replaced by a robust fallback mechanism.
    Conditional checks for microtime(), gettimeofday(), and uniqid() have been removed.

@marc-mabe marc-mabe force-pushed the current_time_wrapper branch 14 times, most recently from c81c27f to 8c044c8 Compare July 23, 2025 13:19
@marc-mabe marc-mabe requested a review from devnexen as a code owner September 23, 2025 06:12
@marc-mabe marc-mabe force-pushed the current_time_wrapper branch from 315f346 to 956b7a0 Compare October 4, 2025 19:57
@marc-mabe marc-mabe changed the title Refactor basic time usages Refactor Internal Time Retrieval Handling for Improved Consistency and Resolution Oct 5, 2025
@marc-mabe marc-mabe force-pushed the current_time_wrapper branch from 956b7a0 to 88ec473 Compare October 5, 2025 05:41
@marc-mabe marc-mabe force-pushed the current_time_wrapper branch from 88ec473 to abf0e46 Compare October 19, 2025 02:53
@marc-mabe marc-mabe requested a review from TimWolla October 19, 2025 03:14
@marc-mabe
Copy link
Contributor Author

marc-mabe commented Oct 19, 2025 •
edited
Loading

@TimWolla I think this is ready to go now. Please could you have another look and/or point me to the right person to ping?

@bukka Please could you help me for how to fix the failing unit test (expecting gettimeofday but now using zend_monotime_fallback)

TimWolla
TimWolla reviewed Oct 19, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Had a rudimentary first look.

TimWolla
TimWolla reviewed Oct 20, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Some more bits. I'm approximately halfway through the PR. Feel free to already make the requested changes with regard to the removal of the macros, this probably saves us both a review cycle.

@marc-mabe
Copy link
Contributor Author

marc-mabe commented Oct 21, 2025

Some more bits. I'm approximately halfway through the PR. Feel free to already make the requested changes with regard to the removal of the macros, this probably saves us both a review cycle.

I'll be able to continue my work on it from tomorrow evening.

@marc-mabe marc-mabe force-pushed the current_time_wrapper branch from 8949f1a to 5dac1fe Compare October 24, 2025 14:33
@marc-mabe
Copy link
Contributor Author

marc-mabe commented Oct 24, 2025

Hi @TimWolla , I have now addressed all your comments.

PS: I decided to not inline zend_time_real_[get|spec] and zend_time_mono_fallback to be able to mock these functions in unit tests.

@marc-mabe marc-mabe force-pushed the current_time_wrapper branch 2 times, most recently from 13a933b to bf46488 Compare November 16, 2025 07:30
@marc-mabe
Copy link
Contributor Author

marc-mabe commented Nov 16, 2025

@TimWolla rebased again - is this good to go now? - Should I squash the commits?

@marc-mabe
Copy link
Contributor Author

marc-mabe commented Dec 18, 2025

@TimWolla It would be awesome if this could be merged

derickr
derickr requested changes Jan 13, 2026
View reviewed changes
@marc-mabe marc-mabe force-pushed the current_time_wrapper branch 2 times, most recently from 371f94e to c3e56a1 Compare January 17, 2026 12:18
@marc-mabe
Refactors internal time retrieval and measurement
e4b56b4 
Introducing a more consistent and portable approach. The primary goals are to
encapsulate platform-specific differences, improve time resolution, and prepare
for long-term compatibility (e.g., Y2038 on WIN64).
@marc-mabe marc-mabe force-pushed the current_time_wrapper branch from c3e56a1 to e4b56b4 Compare January 17, 2026 13:01
@marc-mabe
Copy link
Contributor Author

marc-mabe commented Jan 17, 2026

@derickr @TimWolla I have addressed all your PR comments. Hope it's good to go now.

@marc-mabe marc-mabe requested review from TimWolla and derickr January 17, 2026 13:02
bukka
bukka requested changes Jan 17, 2026
View reviewed changes
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

It looks mostly good but I think this should be split to smaller PR's. It might also help to get those changes merged as preference here should be to get reviews from code owners and if it's as big as this, it's kind of hard to get it.

sapi/fpm/fpm/fpm_unix.c
}
#endif

if (0 > fpm_clock_init()) {
Copy link
Member

@bukka bukka Jan 17, 2026

Choose a reason for hiding this comment

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

What abou that mac specific handling

Copy link
Contributor Author

@marc-mabe marc-mabe Jan 17, 2026

Choose a reason for hiding this comment

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

zend_time_mono_fallback is using zend_hrtime which already handles mac specifics in a better way (like clock_gettime_nsec_np)

sapi/fpm/fpm/fpm_request.c

#include "fpm_config.h"

#include "zend_time.h"
Copy link
Member

@bukka bukka Jan 17, 2026

Choose a reason for hiding this comment

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

I'm not really sure if there is a big benefit in replacing fpm_clock.h here. In general those pieces of FPM should stay independent from Zend code.

Copy link
Contributor Author

@marc-mabe marc-mabe Jan 17, 2026

Choose a reason for hiding this comment

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

The SAPIs are already using Zend code - So they are not independent

Copy link
Member

@bukka bukka Jan 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

FPM has got some parts using Zend but the majority is independent.

@bukka
Copy link
Member

bukka commented Jan 17, 2026

I think this needs a bit deeper review and a bit of research for me to confidentily approve it. It might take me some time. As I noted spliting that to smaller parts might be a better way how to get it merged.

@marc-mabe
Copy link
Contributor Author

marc-mabe commented Jan 17, 2026 •
edited
Loading

@bukka

I think this needs a bit deeper review and a bit of research for me to confidentily approve it. It might take me some time. As I noted spliting that to smaller parts might be a better way how to get it merged.

OK, I'll can split out the SAPI changes for now.

... In general those pieces of FPM should stay independent from Zend code.

What about moving this into main as php_time_* - would that make sense to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bukka bukka bukka requested changes

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

@ndossche ndossche Awaiting requested review from ndossche ndossche is a code owner

@Girgias Girgias Awaiting requested review from Girgias Girgias is a code owner

@zeriyoshi zeriyoshi Awaiting requested review from zeriyoshi zeriyoshi is a code owner

@kamil-tekiela kamil-tekiela Awaiting requested review from kamil-tekiela kamil-tekiela is a code owner

@SakiTakamachi SakiTakamachi Awaiting requested review from SakiTakamachi SakiTakamachi is a code owner

@devnexen devnexen Awaiting requested review from devnexen devnexen is a code owner

@derickr derickr Awaiting requested review from derickr derickr is a code owner

@TimWolla TimWolla Awaiting requested review from TimWolla TimWolla is a code owner

Assignees

No one assigned

Labels

ABI break Category: Build System Category: Engine Category: Optimizer Extension: calendar Extension: date Extension: ftp Extension: mysqlnd Extension: opcache Extension: openssl Extension: pgsql Extension: phar Extension: random Extension: session Extension: soap Extension: standard SAPI: cgi SAPI: cli SAPI: fpm SAPI: litespeed SAPI: phpdbg

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@marc-mabe @bukka @derickr @TimWolla