Everything

php

timelib performance fix

php.internals

Dmitry Stogov

5 years ago

Hi Derick,

Please, let me know you decision according
https://github.com/derickr/timelib/pull/99

This workaround fix makes ~170 times improvement on "new DateTimeZone()"
and as result visible improvement on some real-life apps (e.g Symfony demo
gets ~7% according to callgrind).
This is a huge difference.

The fix was proposed more than a half year ago...
It would be great to include it into PHP-8.1 release.

Thanks. Dmitry.

Pierre Joye

5 years ago

Hi Dmitry,

On Mon, Aug 30, 2021 at 8:54 PM Dmitry Stogov <dmitrystogov@gmail.com> wrote:

> Please, let me know you decision according
> https://github.com/derickr/timelib/pull/99
>
> This workaround fix makes ~170 times improvement on "new DateTimeZone()"
> and as result visible improvement on some real-life apps (e.g Symfony demo
> gets ~7% according to callgrind).
> This is a huge difference.
>
> The fix was proposed more than a half year ago...
> It would be great to include it into PHP-8.1 release.

Such improvements are more than welcome, especially for such obvious
patch. One could argue that the length of the data may change in the
future but it can be increase then, or a macro can define it easily.

It is late in the run to include it, but if RMs are OK, I would be all
for applying it. The lib is bundled and whether the external
repository applies it should not be relevant at this point (also no
activity in 2 years there), or?

Cheers,

--
Pierre

@pierrejoye | http://www.libgd.org

Rowan Collins

5 years ago

On 31/08/2021 07:10, Pierre Joye wrote:

> also no activity in 2 years there

Possibly you looked at the "latest release" in Github's sidebar, which
shows Jan 2019 for some reason, but the most recent tag was actually
three weeks ago: https://github.com/derickr/timelib/tags

I'll leave Derick to comment on the patch itself, since he's the primary
maintainer of both the library in question and the PHP extension that
embeds it.

Regards,

--
Rowan Tommins
[IMSoP]

Ben Ramsey

5 years ago

Pierre Joye wrote on 8/31/21 01:10:

> On Mon, Aug 30, 2021 at 8:54 PM Dmitry Stogov <dmitrystogov@gmail.com> wrote:
>
>> Please, let me know you decision according
>> https://github.com/derickr/timelib/pull/99
>>
>> This workaround fix makes ~170 times improvement on "new DateTimeZone()"
>> and as result visible improvement on some real-life apps (e.g Symfony demo
>> gets ~7% according to callgrind).
>> This is a huge difference.
>>
>> The fix was proposed more than a half year ago...
>> It would be great to include it into PHP-8.1 release.
>
> Such improvements are more than welcome, especially for such obvious
> patch. One could argue that the length of the data may change in the
> future but it can be increase then, or a macro can define it easily.
>
> It is late in the run to include it, but if RMs are OK, I would be all
> for applying it. The lib is bundled and whether the external
> repository applies it should not be relevant at this point (also no
> activity in 2 years there), or?

Is it solely a bug fix and/or performance improvement?

Cheers,
Ben

Kalle Sommer Nielsen

5 years ago

Den tir. 31. aug. 2021 kl. 09.10 skrev Pierre Joye <pierre.php@gmail.com>:

> It is late in the run to include it, but if RMs are OK, I would be all
> for applying it. The lib is bundled and whether the external
> repository applies it should not be relevant at this point (also no
> activity in 2 years there), or?

The external library is important because you can update the pecl
release of that in the middle of a PHP release if an update comes out,
which is good if you are locked to a specific version of PHP. Loading
in the pecl/timelib extension will override the internal version,
however if this fix is only applied to the bundled library, but not
synced upstream, then it breaks this philosophy.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Derick Rethans

5 years ago

Hi Dmitry,

On Mon, 30 Aug 2021, Dmitry Stogov wrote:

> Please, let me know you decision according
> https://github.com/derickr/timelib/pull/99
>
> This workaround fix makes ~170 times improvement on "new DateTimeZone()"
> and as result visible improvement on some real-life apps (e.g Symfony demo
> gets ~7% according to callgrind).
> This is a huge difference.
>
> The fix was proposed more than a half year ago...
> It would be great to include it into PHP-8.1 release.

I'd seen the original PR, which still had "changes requested" open, but
I've had a look now and instead of picking an arbitrary limit, we're
actually respecting the POSIX system limit (which is 6, usually).

Merged into PHP's master (among another bug fix) too.

cheers,
Derick

--
PHP 7.4 Release Manager
Host of PHP Internals News: https://phpinternals.news
Like Xdebug? Consider supporting me: https://xdebug.org/support
https://derickrethans.nl | https://xdebug.org | https://dram.io
twitter: @derickr and @xdebug