Unnecessary overhead in getdate()

php.internals

Benjamin Coutu

8 years ago
Hello, Just like `[i]date()`, `getdate()` without the `$timestamp` argument uses the current timestamp. The code for `[i]date()` (https://github.com/php/php-src/blob/master/ext/date/php_date.c#L1276) only makes a system call to `time(NULL)` when the `$timestamp` argument is missing. In contrast `getdate()` (https://github.com/php/php-src/blob/master/ext/date/php_date.c#L1828) always performs a system call to `time(NULL)` even if `$timestamp` is supplied by the user. The code for `getdate()` should mimic what is done for `date()` to avoid unnecessary system call overhead, hence only call `time(NULL)` when `!ZEND_NUM_ARGS()`. Can someone please make the appropriate change? Please apologize, but I simply review code and do not have a proper C development setup that allows me to supply patches or create pull requests. Thanks, Ben -- Benjamin Coutu http://www.zeyos.com

Christoph Becker

8 years ago
On 29.03.2018 at 11:51, Benjamin Coutu wrote:
> Just like `[i]date()`, `getdate()` without the `$timestamp` argument uses the current timestamp. > > The code for `[i]date()` (https://github.com/php/php-src/blob/master/ext/date/php_date.c#L1276) only makes a system call to `time(NULL)` when the `$timestamp` argument is missing. In contrast `getdate()` (https://github.com/php/php-src/blob/master/ext/date/php_date.c#L1828) always performs a system call to `time(NULL)` even if `$timestamp` is supplied by the user. > > The code for `getdate()` should mimic what is done for `date()` to avoid unnecessary system call overhead, hence only call `time(NULL)` when `!ZEND_NUM_ARGS()`.
ACK. I wonder, though, whether it is safe to convert a `time_t` (which is supposed to be returned by `time()`) to `zend_long`. POSIX 6 claims that `time_t` shall be an integer or real-floating type[1]; POSIX 7 is more specific ("time_t shall be an integer type")[2], but obviously there are neither guarantees regarding signess nor size, and it doesn't seem that we have respective checks in place[3]. [1] <http://pubs.opengroup.org/onlinepubs/009695299/basedefs/sys/types.h.html> [2] <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html> [3] <https://github.com/php/php-src/blob/master/configure.ac>
-- Christoph M. Becker