[PHP-DEV] timelib performance fix

  115897
August 30, 2021 13:53 dmitrystogov@gmail.com (Dmitry Stogov)
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.
  115900
August 31, 2021 06:10 pierre.php@gmail.com (Pierre Joye)
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
  115901
August 31, 2021 13:52 rowan.collins@gmail.com (Rowan Tommins)
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]
  115907
August 31, 2021 20:58 ramsey@php.net (Ben Ramsey)
--or2QY8liKfckqNeLe8VNdstLwKuCDWE9x
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

Pierre Joye wrote on 8/31/21 01:10:
> On Mon, Aug 30, 2021 at 8:54 PM Dmitry Stogov <dmitrystogov@gmail.com> = wrote:
>=20 >> 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. >=20 > 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. >=20 > 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 --or2QY8liKfckqNeLe8VNdstLwKuCDWE9x--
  115908
August 31, 2021 21:18 kalle@php.net (Kalle Sommer Nielsen)
Den tir. 31. aug. 2021 kl. 09.10 skrev Pierre Joye 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
  115905
August 31, 2021 14:32 derick@php.net (Derick Rethans)
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