revert #30525 due to performance penalty #32165
Conversation
|
Master then is 4.3 now so yes, this should target 4.3. But can we have a deeper analysis so that we better understand what's going on? The reverted PR had merits I suppose. |
|
I'm in favor of reverting in the meantime, this change looks to impact badly most API Platform projects. |
|
From the profile, it seems the reverted PR caused many many cache lookups and writes. |
|
Thank you @bendavies. |
This PR was merged into the 4.3 branch. Discussion ---------- revert #30525 due to performance penalty | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | This reverts #30525, because it introduced a large performance penalty. Here is the effect of that PR when I upgrade my Api Platform project from 4.2.9 to 4.3.1: https://blackfire.io/profiles/compare/28bfbc61-3649-4896-bd03-7201239134cd/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=Symfony%5CComponent%5CVarExporter%5CInternal%5CExporter%3A%3Aexport%403&callname=main() The reverted PR targeted master. This should go in 4.3? Commits ------- 3d37cc9 revert #30525 due to performance penalty
|
THANKS! FYI, a cache warmup with symfony 4.3 with apip : https://blackfire.io/profiles/d20d4c31-f68c-406a-b691-94dba2422d6b/graph. With both this problem and this one #32188. Yep, 16mn cache warmup... 14 for this one and 1.5 for the new PR (#32188).
|
|
From the original issue I filed at #29977:
Do we have any idea if either one of these cases is what's happened? In our case, it's the opposite, and without the original PR we see additional seconds in latency. It'd be great if we could figure out something that handles both cases. |
This reverts #30525, because it introduced a large performance penalty.
Here is the effect of that PR when I upgrade my Api Platform project from 4.2.9 to 4.3.1:
https://blackfire.io/profiles/compare/28bfbc61-3649-4896-bd03-7201239134cd/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=Symfony%5CComponent%5CVarExporter%5CInternal%5CExporter%3A%3Aexport%403&callname=main()
The reverted PR targeted master. This should go in 4.3?