Replace spl_object_hash() with spl_object_id() #8837

greg0ire · 2021-07-10T09:37:58Z

It is more efficient, and can be provided to 7.1 users thanks to
symfony/polyfill-php72.

Backports #6878

@beberlei I know we wanted to target Doctrine 3 with this, but I found a way to target v2. WDYT?

It is more efficient, and can be provided to 7.1 users thanks to symfony/polyfill-php72.

greg0ire · 2021-07-10T09:59:28Z

Comparative benchmark (based on https://phpbench.readthedocs.io/en/latest/guides/regression-testing.html):

PHPBench (1.0.3) running benchmarks...
with configuration file: /home/greg/dev/doctrine-orm/phpbench.json
with PHP version 7.4.20, xdebug ❌, opcache ❌
comparing [actual vs. original]

\Doctrine\Performance\Hydration\SingleTableInheritanceInsertPerformanceBench

    benchInsertFixContracts.................I9 - [Mo5.030ms vs. Mo5.145ms] -2.23% (±0.97%)
    benchInsertFlexContracts................I9 - [Mo5.063ms vs. Mo5.198ms] -2.59% (±1.12%)
    benchInsertUltraContracts...............I9 - [Mo5.163ms vs. Mo5.285ms] -2.31% (±0.56%)
    benchInsertAllContracts.................R2 I7 - [Mo7.079ms vs. Mo7.470ms] -5.24% (±1.33%)

\Doctrine\Performance\Hydration\MixedQueryFetchJoinFullObjectHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo75.851ms vs. Mo80.428ms] -5.69% (±0.46%)

\Doctrine\Performance\Hydration\SimpleQueryPartialObjectHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo76.042ms vs. Mo81.396ms] -6.58% (±0.52%)

\Doctrine\Performance\Hydration\SimpleHydrationBench

    benchHydration..........................R2 I9 - [Mo192.515ms vs. Mo198.039ms] -2.79% (±0.65%)

\Doctrine\Performance\Hydration\SingleTableInheritanceHydrationPerformanceBench

    benchHydrateFixContracts................R2 I9 - [Mo2.546ms vs. Mo2.547ms] -0.07% (±0.92%)
    benchHydrateFlexContracts...............R2 I9 - [Mo3.132ms vs. Mo3.154ms] -0.69% (±0.77%)
    benchHydrateUltraContracts..............R2 I9 - [Mo2.647ms vs. Mo2.661ms] -0.53% (±1.14%)
    benchHydrateAllContracts................R2 I9 - [Mo3.590ms vs. Mo3.672ms] -2.25% (±0.83%)

\Doctrine\Performance\Hydration\MixedQueryFetchJoinArrayHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo76.936ms vs. Mo77.214ms] -0.36% (±0.41%)

\Doctrine\Performance\Hydration\SimpleQueryScalarHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo24.525ms vs. Mo24.716ms] -0.77% (±0.16%)

\Doctrine\Performance\Hydration\SimpleInsertPerformanceBench

    benchHydration..........................R2 I9 - [Mo43.019ms vs. Mo56.257ms] -23.53% (±0.45%)

\Doctrine\Performance\Hydration\MixedQueryFetchJoinPartialObjectHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo44.334ms vs. Mo47.313ms] -6.29% (±0.38%)

\Doctrine\Performance\Hydration\SimpleQueryArrayHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo38.163ms vs. Mo38.151ms] +0.03% (±0.43%)

\Doctrine\Performance\Hydration\SimpleQueryFullObjectHydrationPerformanceBench

    benchHydration..........................R2 I9 - [Mo217.158ms vs. Mo233.919ms] -7.17% (±0.45%)

\Doctrine\Performance\ChangeSet\UnitOfWorkComputeChangesBench

    benchChangeSetComputation...............R2 I9 - [Mo946.411μs vs. Mo1.022ms] -7.41% (±1.01%)

\Doctrine\Performance\Query\QueryBoundParameterProcessingBench

    benchExecuteParsedQueryWithInferredPara.R2 I9 - [Mo2.075ms vs. Mo2.075ms] -0.03% (±1.57%)
    benchExecuteParsedQueryWithDeclaredPara.R2 I9 - [Mo1.407ms vs. Mo1.420ms] -0.90% (±0.59%)

\Doctrine\Performance\LazyLoading\ProxyInitializationTimeBench

    benchCmsUserInitialization..............R2 I9 - [Mo35.619ms vs. Mo35.691ms] -0.20% (±0.36%)
    benchCmsEmployeeInitialization..........R2 I9 - [Mo6.374ms vs. Mo6.393ms] -0.29% (±0.47%)
    benchInitializationOfAlreadyInitialized.R2 I9 - [Mo478.998μs vs. Mo490.419μs] -2.33% (±1.50%)
    benchInitializationOfAlreadyInitialized.R2 I9 - [Mo384.131μs vs. Mo385.025μs] -0.23% (±1.36%)

\Doctrine\Performance\LazyLoading\ProxyInstantiationTimeBench

    benchCmsUserInstantiation...............R2 I9 - [Mo83.374ms vs. Mo84.546ms] -1.39% (±0.55%)
    benchCmsEmployeeInstantiation...........R2 I9 - [Mo49.536ms vs. Mo49.578ms] -0.08% (±0.07%)

Subjects: 26, Assertions: 0, Failures: 0, Errors: 0

+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+
| benchmark                                                 | subject                                             | set | revs | its | mem_peak         | mode                 | rstdev          |
+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+
| SingleTableInheritanceInsertPerformanceBench              | benchInsertFixContracts                             | 0   | 1    | 10  | 6.739mb -0.39%   | 5,029.626μs -2.23%   | ±0.97% +17.72%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertFlexContracts                            | 0   | 1    | 10  | 6.752mb -0.39%   | 5,063.096μs -2.59%   | ±1.12% -53.83%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertUltraContracts                           | 0   | 1    | 10  | 6.797mb -0.39%   | 5,163.299μs -2.31%   | ±0.56% +21.56%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertAllContracts                             | 0   | 1    | 10  | 7.126mb -1.20%   | 7,078.806μs -5.24%   | ±1.33% +138.22% |
| MixedQueryFetchJoinFullObjectHydrationPerformanceBench    | benchHydration                                      | 0   | 1    | 10  | 18.880mb -5.77%  | 75,851.096μs -5.69%  | ±0.46% -2.16%   |
| SimpleQueryPartialObjectHydrationPerformanceBench         | benchHydration                                      | 0   | 1    | 10  | 26.541mb -9.64%  | 76,042.472μs -6.58%  | ±0.52% +35.38%  |
| SimpleHydrationBench                                      | benchHydration                                      | 0   | 1    | 10  | 68.928mb -14.85% | 192,514.935μs -2.79% | ±0.65% -24.38%  |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateFixContracts                            | 0   | 1    | 10  | 7.184mb -0.12%   | 2,545.632μs -0.07%   | ±0.92% -8.69%   |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateFlexContracts                           | 0   | 1    | 10  | 7.328mb -0.24%   | 3,131.945μs -0.69%   | ±0.77% +85.91%  |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateUltraContracts                          | 0   | 1    | 10  | 7.205mb -0.12%   | 2,647.366μs -0.53%   | ±1.14% -43.99%  |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateAllContracts                            | 0   | 1    | 10  | 7.404mb -0.35%   | 3,589.687μs -2.25%   | ±0.83% -26.18%  |
| MixedQueryFetchJoinArrayHydrationPerformanceBench         | benchHydration                                      | 0   | 1    | 10  | 32.582mb -0.00%  | 76,935.849μs -0.36%  | ±0.41% +17.12%  |
| SimpleQueryScalarHydrationPerformanceBench                | benchHydration                                      | 0   | 1    | 10  | 15.232mb -0.00%  | 24,524.886μs -0.77%  | ±0.16% -56.03%  |
| SimpleInsertPerformanceBench                              | benchHydration                                      | 0   | 1    | 10  | 18.733mb -25.59% | 43,019.389μs -23.53% | ±0.45% -62.12%  |
| MixedQueryFetchJoinPartialObjectHydrationPerformanceBench | benchHydration                                      | 0   | 1    | 10  | 13.005mb -4.45%  | 44,334.419μs -6.29%  | ±0.38% -23.28%  |
| SimpleQueryArrayHydrationPerformanceBench                 | benchHydration                                      | 0   | 1    | 10  | 15.113mb -0.00%  | 38,163.403μs +0.03%  | ±0.43% +32.23%  |
| SimpleQueryFullObjectHydrationPerformanceBench            | benchHydration                                      | 0   | 1    | 10  | 63.384mb -8.08%  | 217,158.241μs -7.17% | ±0.45% -29.74%  |
| UnitOfWorkComputeChangesBench                             | benchChangeSetComputation                           | 0   | 1    | 10  | 5.767mb -0.89%   | 946.411μs -7.41%     | ±1.01% +71.72%  |
| QueryBoundParameterProcessingBench                        | benchExecuteParsedQueryWithInferredParameterType    | 0   | 1    | 10  | 6.044mb -0.00%   | 2,074.685μs -0.03%   | ±1.57% +21.43%  |
| QueryBoundParameterProcessingBench                        | benchExecuteParsedQueryWithDeclaredParameterType    | 0   | 1    | 10  | 6.014mb -0.00%   | 1,407.170μs -0.90%   | ±0.59% -65.39%  |
| ProxyInitializationTimeBench                              | benchCmsUserInitialization                          | 0   | 1    | 10  | 17.812mb -0.00%  | 35,619.329μs -0.20%  | ±0.36% -25.70%  |
| ProxyInitializationTimeBench                              | benchCmsEmployeeInitialization                      | 0   | 1    | 10  | 17.812mb -0.00%  | 6,373.785μs -0.29%   | ±0.47% -4.93%   |
| ProxyInitializationTimeBench                              | benchInitializationOfAlreadyInitializedCmsUsers     | 0   | 1    | 10  | 17.812mb -0.00%  | 478.998μs -2.33%     | ±1.50% -13.63%  |
| ProxyInitializationTimeBench                              | benchInitializationOfAlreadyInitializedCmsEmployees | 0   | 1    | 10  | 17.812mb -0.00%  | 384.131μs -0.23%     | ±1.36% -34.58%  |
| ProxyInstantiationTimeBench                               | benchCmsUserInstantiation                           | 0   | 1    | 10  | 5.758mb -0.00%   | 83,374.440μs -1.39%  | ±0.55% -26.37%  |
| ProxyInstantiationTimeBench                               | benchCmsEmployeeInstantiation                       | 0   | 1    | 10  | 5.604mb -0.00%   | 49,536.205μs -0.08%  | ±0.07% -77.13%  |
+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+

It can be easier to interpret with some color:

I think the differences in standard deviation can be ignored, and I notice that the result is consistently positive, especially for SimpleInsertPerformanceBench

To reproduce it locally, try this:

git switch --detach origin/2.10.x
vendor/bin/phpbench run --tag=original --retry-threshold=5 --iterations=10
git switch backport-6878
vendor/bin/phpbench run  --report=aggregate --ref=original --retry-threshold=5 --iterations=10

greg0ire · 2021-07-10T10:34:56Z

composer.json

@@ -32,6 +32,7 @@
        "doctrine/persistence": "^2.2",
        "psr/cache": "^1 || ^2 || ^3",
        "symfony/console": "^3.0 || ^4.0 || ^5.0 || ^6.0",
+        "symfony/polyfill-php72": "^1.23",


This is in order to polyfill spl_object_id: https://github.com/symfony/polyfill-php72/blob/9a142215a36a3888e30d0a9eeea9766764e96976/Php72.php#L89-L100

beberlei · 2021-07-10T14:37:16Z

This is a great idea via polyfill!

greg0ire · 2021-07-11T14:25:27Z

With more revolutions and iterations:

+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+
| benchmark                                                 | subject                                             | set | revs | its | mem_peak         | mode                 | rstdev          |
+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+
| SingleTableInheritanceInsertPerformanceBench              | benchInsertFixContracts                             | 0   | 100  | 10  | 8.207mb -0.33%   | 242.094μs -15.03%    | ±0.57% +13.20%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertFlexContracts                            | 0   | 100  | 10  | 8.207mb -0.33%   | 276.206μs -13.13%    | ±0.38% -39.58%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertUltraContracts                           | 0   | 100  | 10  | 8.241mb -0.33%   | 286.597μs -13.40%    | ±0.72% +15.09%  |
| SingleTableInheritanceInsertPerformanceBench              | benchInsertAllContracts                             | 0   | 100  | 10  | 11.853mb -0.65%  | 787.659μs -14.61%    | ±0.79% +101.73% |
| MixedQueryFetchJoinFullObjectHydrationPerformanceBench    | benchHydration                                      | 0   | 100  | 10  | 18.880mb -5.77%  | 3.075μs +1.55%       | ±0.83% -8.28%   |
| SimpleQueryPartialObjectHydrationPerformanceBench         | benchHydration                                      | 0   | 100  | 10  | 26.541mb -9.64%  | 1.646μs -0.90%       | ±1.31% +36.41%  |
| SimpleHydrationBench                                      | benchHydration                                      | 0   | 100  | 10  | 68.928mb -14.85% | 100,278.948μs -5.82% | ±1.06% +77.28%  |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateFixContracts                            | 0   | 100  | 10  | 7.184mb -0.12%   | 222.851μs -6.96%     | ±1.08% +39.95%  |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateFlexContracts                           | 0   | 100  | 10  | 7.328mb -0.24%   | 477.973μs -4.77%     | ±0.89% +3.97%   |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateUltraContracts                          | 0   | 100  | 10  | 7.205mb -0.12%   | 255.253μs -6.93%     | ±0.79% -6.71%   |
| SingleTableInheritanceHydrationPerformanceBench           | benchHydrateAllContracts                            | 0   | 100  | 10  | 7.404mb -0.35%   | 692.490μs -8.03%     | ±0.87% -17.86%  |
| MixedQueryFetchJoinArrayHydrationPerformanceBench         | benchHydration                                      | 0   | 100  | 10  | 32.582mb -0.00%  | 1.080μs -0.37%       | ±0.94% +2.36%   |
| SimpleQueryScalarHydrationPerformanceBench                | benchHydration                                      | 0   | 100  | 10  | 15.232mb -0.00%  | 0.870μs -0.06%       | ±0.00% -100.00% |
| SimpleInsertPerformanceBench                              | benchHydration                                      | 0   | 100  | 10  | 18.733mb -25.59% | 35,425.241μs -15.53% | ±0.19% -50.93%  |
| MixedQueryFetchJoinPartialObjectHydrationPerformanceBench | benchHydration                                      | 0   | 100  | 10  | 13.005mb -4.45%  | 2.631μs +0.65%       | ±0.75% -24.87%  |
| SimpleQueryArrayHydrationPerformanceBench                 | benchHydration                                      | 0   | 100  | 10  | 15.113mb -0.00%  | 1.019μs +0.75%       | ±0.65% -45.28%  |
| SimpleQueryFullObjectHydrationPerformanceBench            | benchHydration                                      | 0   | 100  | 10  | 63.384mb -8.08%  | 2.571μs +0.25%       | ±0.99% -1.11%   |
| UnitOfWorkComputeChangesBench                             | benchChangeSetComputation                           | 0   | 100  | 10  | 5.767mb -0.90%   | 841.348μs -7.11%     | ±0.22% -50.59%  |
| QueryBoundParameterProcessingBench                        | benchExecuteParsedQueryWithInferredParameterType    | 0   | 100  | 10  | 6.044mb -0.00%   | 90.177μs +0.08%      | ±0.70% -7.26%   |
| QueryBoundParameterProcessingBench                        | benchExecuteParsedQueryWithDeclaredParameterType    | 0   | 100  | 10  | 6.014mb -0.00%   | 13.696μs -0.15%      | ±0.67% +15.81%  |
| ProxyInitializationTimeBench                              | benchCmsUserInitialization                          | 0   | 100  | 10  | 17.812mb -0.00%  | 271.198μs -4.72%     | ±0.91% -26.54%  |
| ProxyInitializationTimeBench                              | benchCmsEmployeeInitialization                      | 0   | 100  | 10  | 17.812mb -0.00%  | 264.560μs -0.14%     | ±0.37% -45.58%  |
| ProxyInitializationTimeBench                              | benchInitializationOfAlreadyInitializedCmsUsers     | 0   | 100  | 10  | 17.812mb -0.00%  | 274.695μs +1.37%     | ±0.87% +9.04%   |
| ProxyInitializationTimeBench                              | benchInitializationOfAlreadyInitializedCmsEmployees | 0   | 100  | 10  | 17.812mb -0.00%  | 263.860μs -0.06%     | ±0.38% -44.05%  |
| ProxyInstantiationTimeBench                               | benchCmsUserInstantiation                           | 0   | 100  | 10  | 5.758mb -0.00%   | 68,777.725μs -1.43%  | ±0.63% +105.20% |
| ProxyInstantiationTimeBench                               | benchCmsEmployeeInstantiation                       | 0   | 100  | 10  | 5.604mb -0.00%   | 41,933.617μs -0.28%  | ±0.10% -83.68%  |
+-----------------------------------------------------------+-----------------------------------------------------+-----+------+-----+------------------+----------------------+-----------------+

The results are similar memory-wise, cpu-wise it's less good

mvorisek · 2021-07-12T17:11:08Z

👍 , since https://github.com/php/php-src/pull/7010/files#diff-43d1339545d5d900325c0ce9a1285f63dbee8a203559fa5893128b37cdbc9c37R660 spl_object_hash function has really no advantage over spl_object_id and it may even be removed from php in the future

SenseException · 2021-07-13T22:28:24Z

7.1 is the only version that will use the polyfill, and I don't expect a bigger negative benchmark impact there. It uses spl_object_hash under the hood. PHP 7.1 is outdated anyway.

greg0ire · 2021-07-13T22:37:38Z

I don't know if we have a list of reasons of dropping PHP 7.1 but that would certainly add to it. In fact, it's the second time in a short timeframe that it's suggested: #8838 (comment) . Maybe we should reconsider it, but that's another story.

beberlei · 2021-07-13T22:45:48Z

@greg0ire reconsider how? Both issues are related to spl_object_id which csn be polyfilled as you showed. We should drop 7.1 with ORM 3.0

greg0ire · 2021-07-14T08:33:05Z

reconsider how

By compiling a list of benefits mentioned here and there, and by taking a look at the newly available Packagist stats: https://packagist.org/packages/doctrine/orm/php-stats . Maybe we should even have a policy about dropping versions of PHP based on these stats, since I don't think waiting for 4.0 to drop 7.2 is going to be reasonable. In the meantime, can either of you please approve this PR?

alcaeus · 2021-07-21T00:38:40Z

@greg0ire reconsider how? Both issues are related to spl_object_id which csn be polyfilled as you showed. We should drop 7.1 with ORM 3.0

At the rate we're going, we should at least aim to drop PHP 7.3.

stof · 2021-11-17T14:30:48Z

lib/Doctrine/ORM/UnitOfWork.php

@@ -3241,7 +3241,7 @@ public function registerManaged($entity, array $id, array $data)
     * INTERNAL:
     * Clears the property changeset of the entity with the given OID.
     *
-     * @param string $oid The entity's OID.
+     * @param int $oid The entity's OID.


This was actually a BC break regarding gedmo/doctrine-extensions (they adapted the code in doctrine-extensions/DoctrineExtensions#2272 though, so no need to revert it as a second BC break).
this method has INTERNAL: in its description, but it is not tagged as @internal so using it in third-party code is not flagged. And it looks like there is no public API allowing to achieve that goal.

OK, I see that #9143 actually documents it

greg0ire added the Backporting label Jul 10, 2021

Replace spl_object_hash() with spl_object_id()

84ad007

It is more efficient, and can be provided to 7.1 users thanks to symfony/polyfill-php72.

greg0ire force-pushed the backport-6878 branch from fae6686 to 84ad007 Compare July 10, 2021 09:55

greg0ire requested review from beberlei and SenseException July 10, 2021 10:13

greg0ire commented Jul 10, 2021

View reviewed changes

greg0ire mentioned this pull request Jul 11, 2021

Remove expensive array ops #8838

Merged

beberlei approved these changes Jul 20, 2021

View reviewed changes

greg0ire merged commit ccc2993 into doctrine:2.10.x Jul 20, 2021

greg0ire deleted the backport-6878 branch July 20, 2021 21:19

greg0ire added this to the 2.10.0 milestone Jul 20, 2021

greg0ire mentioned this pull request Aug 27, 2021

DDC-2332: [UnitOfWork::doPersist()] The spl_object_hash() generate not unique hash! #3037

Closed

github-actions bot mentioned this pull request Oct 3, 2021

Merge release 2.10.0 into 3.0.x #9066

Merged

This was referenced Oct 21, 2021

Allow ORM 2.10.2 and later doctrine-extensions/DoctrineExtensions#2271

Closed

spl_object_hash() vs spl_object_id() #9141

Closed

greg0ire mentioned this pull request Oct 21, 2021

Document BC break #9143

Merged

stof reviewed Nov 17, 2021

View reviewed changes

franmomu mentioned this pull request Jan 5, 2022

Saving a field in another locale override that field, losing data. Any kind of help is much appreciated. sonata-project/SonataTranslationBundle#615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace spl_object_hash() with spl_object_id() #8837

Replace spl_object_hash() with spl_object_id() #8837

greg0ire commented Jul 10, 2021

greg0ire commented Jul 10, 2021 •

edited

Loading

greg0ire Jul 10, 2021

beberlei commented Jul 10, 2021

greg0ire commented Jul 11, 2021 •

edited

Loading

mvorisek commented Jul 12, 2021

SenseException commented Jul 13, 2021

greg0ire commented Jul 13, 2021

beberlei commented Jul 13, 2021

greg0ire commented Jul 14, 2021

alcaeus commented Jul 21, 2021

stof Nov 17, 2021

stof Nov 17, 2021

Replace spl_object_hash() with spl_object_id() #8837

Replace spl_object_hash() with spl_object_id() #8837

Conversation

greg0ire commented Jul 10, 2021

greg0ire commented Jul 10, 2021 • edited Loading

greg0ire Jul 10, 2021

Choose a reason for hiding this comment

beberlei commented Jul 10, 2021

greg0ire commented Jul 11, 2021 • edited Loading

mvorisek commented Jul 12, 2021

SenseException commented Jul 13, 2021

greg0ire commented Jul 13, 2021

beberlei commented Jul 13, 2021

greg0ire commented Jul 14, 2021

alcaeus commented Jul 21, 2021

stof Nov 17, 2021

Choose a reason for hiding this comment

stof Nov 17, 2021

Choose a reason for hiding this comment

greg0ire commented Jul 10, 2021 •

edited

Loading

greg0ire commented Jul 11, 2021 •

edited

Loading