-
Notifications
You must be signed in to change notification settings - Fork 8k
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.
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
base: master
Are you sure you want to change the base?
Conversation
c81c27f
to
8c044c8
Compare
7e3d98c
to
fdabd9b
Compare
1058493
to
9429859
Compare
9429859
to
3aa094c
Compare
315f346
to
956b7a0
Compare
956b7a0
to
88ec473
Compare
88ec473
to
abf0e46
Compare
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make all the macros that could be (inline) functions a proper function? I think that should be all of them.
Please also properly “namespace” all the symbols by prefixing them with zend_time_*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make all the macros that could be (inline) functions a proper function? I think that should be all of them.
👍
Please also properly “namespace” all the symbols by prefixing them with zend_time_*.
For the zend_realtime_spec
, zend_realtime_get
and zend_monotime_fallback
functions I felt it sounds more natural and it's obvious these functions belong to time.
Would zend_time_real_get
, zend_time_real_spec
and zend_time_mono_fallback
better match with your expected naming scheme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
zend_time_real_get
,zend_time_real_spec
andzend_time_mono_fallback
better match with your expected naming scheme?
Yes. While it might sound a little awkward, I find it valuable to easily locate where the function is declared.
ZEND_API void zend_realtime_spec(struct timespec *ts) { | ||
#if defined(HAVE_CLOCK_GETTIME) | ||
|
||
clock_gettime(CLOCK_REALTIME, ts); | ||
|
||
#elif defined(HAVE_TIMESPEC_GET) | ||
|
||
timespec_get(ts, TIME_UTC); | ||
|
||
#elif defined(HAVE_GETTIMEOFDAY) | ||
|
||
struct timeval tv; | ||
gettimeofday(&tv, NULL); | ||
zend_time_val2spec(tv, ts); | ||
|
||
#else | ||
|
||
ts->tv_sec = zend_realtime_get(); | ||
ts->tv_nsec = 0; | ||
|
||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these all infallible at runtime? Complexity-wise this could also be an inline function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these functions can return an error in case of using wrong/unsupported arguments.
On the same time here only the most basic functionality is used and in the previous implementation error returns where not checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the previous implementation error returns where not checked.
Sure, but I don't think this is a reason that this should not be improved. Though it certainly raises the question if a broken clock is even recoverable.
Perhaps add a (void)
in front to make it explicit that we don't want to handle the errors?
There was a problem hiding this 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.
time->us = microsecond; | ||
} | ||
|
||
static void php_date_get_current_time_with_fraction(time_t *sec, suseconds_t *usec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one can probably be removed entirely now.
|
||
PHPAPI time_t php_time(void); | ||
/* DEPRECATED, use zend_realtime_get() instead */ | ||
#define php_time() zend_realtime_get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please leave this as a (inline) function and add ZEND_ATTRIBUTE_DEPRECATED
so that folks actually notice that this is deprecated?
I'll be able to continue my work on it from tomorrow evening. |
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_realtime_spec
A unified wrapper around
clock_gettime()
,timespec_get()
,gettimeofday()
, andtime()
using the real/wall clock.Returns a
timespec
structure with nanosecond precision (when available).Defined
zend_realtime_get
macroA lightweight macro wrapping
time(NULL)
for simple, low-resolution time retrieval.Introduced
zend_monotime_fallback
A macro that maps to
zend_hrtime
or falls back tozend_realtime_spec
.Useful for time measurements (e.g. timeout handling) where monotonic time is preferred, but wall time is an acceptable fallback.
Provided helper macros
Added utilities to simplify usage of
timeval
andtimespec
structures.Replaced direct system time API usage
Internal calls to system time functions now use
zend_realtime_*
orzend_monotime_fallback
.Standardized time representation
Transitioned to
timespec
for current time where appropriate, while retainingtimeval
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
intimeval.tv_sec
on 64-bit Windows systems.Improved Resolution for Time Functions
microtime()
andgettimeofday(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()
, anduniqid()
have been removed.