Huge serialize performance degradation Carbon >= 2.61.0

[zeriyoshi]

Hello, Since 2.61.0, Carbon seems to be causing significant performance degradation in the serialization and unserialization process.

This can be reproduced with the following code and commands

<?php declare(strict_types=1);

use const DIRECTORY_SEPARATOR as DS;

require __DIR__ . DS . 'vendor' . DS . 'autoload.php';

const ITER = 100000;

$carbon = new \Carbon\Carbon('now', 'Asia/Tokyo');

$start_at = \hrtime(\true);
for ($i = 0; $i < ITER; $i++) {
    \unserialize(\serialize($carbon));
}
$end_at = \hrtime(\true);

$elapsed = $end_at - $start_at;

echo "runtime version: ", \PHP_VERSION . "\n";
echo "nesbot/carbon: " . \Composer\InstalledVersions::getVersion('nesbot/carbon') . "\n";
echo "elapsed: {$elapsed}ns\n";
echo "per iteration: " . ($elapsed / ITER) . "ns\n";

$ composer require nesbot/carbon:~2.61.0 --quiet && php bench.php 
runtime version: 8.1.18
nesbot/carbon: 2.61.0.0
elapsed: 512420709ns
per iteration: 5124.20709ns

$ composer require nesbot/carbon:~2.60.0 --quiet && php bench.php 
runtime version: 8.1.18
nesbot/carbon: 2.60.0.0
elapsed: 181720459ns
per iteration: 1817.20459ns

Carbon version: 2.60.0, 2.61.0 or later
PHP version: PHP 8.1.18

Thanks!

[taka-oyama]

Probably caused by __serialize and __unserialize override on this commit.

#2644

[kylekatarnls]

Hello, I will inspect possible improvements, however, there is little chances that we can make it as good as before. Previously (in PHP 8.1) we was able to use the native serialization (so optimized C-coded algorithm from PHP itself). Since PHP 8.2 removed feature that broke the serialization, we have no choice but to rewrite it in PHP.

Also being compatible with the last stable version of PHP is prior to optimization for older versions and I can't afford the maintenance burden to have different versions of the library for specific versions of PHP.

[bartabel]

Hello, I will inspect possible improvements, however, there is little chances that we can make it as good as before. Previously (in PHP 8.1) we was able to use the native serialization (so optimized C-coded algorithm from PHP itself). Since PHP 8.2 removed feature that broke the serialization, we have no choice but to rewrite it in PHP.

Hi @kylekatarnls, can you explain what is exactly broken in the native serialization feature? This can help us in deciding how to manage the impact of this issue.

[kylekatarnls]

You can remove __serialize and __unserialize and run with PHP 8.2:

$reflection = (new ReflectionClass(Carbon::class))->newInstanceWithoutConstructor();

@$reflection->date = '1990-01-17 10:28:07';
@$reflection->timezone_type = 3;
@$reflection->timezone = 'US/Pacific';

$date = unserialize(serialize($reflection));

IIRC, this was to support https://packagist.org/packages/opis/closure #2621.

Because the code above works with DateTime and PHP 8.2 and Carbon is supposed to be compatible with DateTime parent class as much as possible, the serialization override were added to support this way to construct instances.

However it sounds it's no longer possible with PHP 8.3 (even with raw DateTime object) so possibly we could run it only for PHP 8.2 and get rid of most of the performance penalty.

Another point is that by default it will serialize only the DateTime properties (date and timezone) so the extra state living in Carbon (such as the local language) are lost in serialization with the default method.

[kylekatarnls]

Another pain point is that __serialize/__unserialize are also ensuring the compatibility of msgpack extension (see #2299) for PHP 8.

I just tried to remove it again to see if msgpack fixed the issue from their side. But I run into a segfault error (not sure if it's related):

master...refactor/serialization (See the GitHub Actions result of the commit 8746237)

I will add few tests to demonstrate the serialization steps that we want to stay backward-compatible, then the goal will be to have all the versions of PHP green. Only at this condition we could get rid of (completely or partially) of the __serialize/__unserialize override / or merge performance optimization.

[taka-oyama]

msgpack 2.2.0 was released recently with the following fix.

Add support for ZEND_ACC_NOT_SERIALIZABLE and magic __{,un}serialize

Could this have fixed part of the issue?

[kylekatarnls]

Very likely as locally all the tests are passing and the "debug" step in GitHub Actions is also passing.

But I can't explain why it segfault in a test that sounds completely unrelated when we remove the __serialize override. It sounds like default DateTime serialization might have issues that our override has not.

Also there are 2 issues with PHP >= 8.2 that I got locally:
Tests\Carbon\SettersTest::testTimeZoneOfUnserialized fails line 297
=> setting timezone broken from an unserialized object
Tests\Carbon\SerializationTest::testSerialization fails line 72
=> locale lost in serialization

[zeriyoshi]

I will inspect this issue using the debug built PHP at hand. Perhaps this is a bug in PHP itself.

[zeriyoshi]

bug reported: php/php-src#11455

[kylekatarnls]

I think they won't handle it, they usually require to have a minimum reproducible case with raw PHP code, without library dependencies.

[zeriyoshi]

Yes, I am trying to create a case to reproduce it at a minimum :)
Let me know if you can reproduce it.