Closed beberlei closed 2 years ago
I think this mostly needs documentation improvements:
laminas/*
packages is intentional, and can only be mitigated by removal of the requirement in the next major release: this is because anyone requiring zendframework/*
packages (and packages in its ecosystem) will get compatible laminas/*
packages, and we cannot just break those until a new major release of the laminas/*
packages is here. This is not clear, and multiple people have asked about it."replace": {"laminas/laminas-zendframework-bridge": "*"}
if you are running within an environment where you no longer rely on zendframework/*
packages. This needs to be better documented, or perhaps added to the output of composer install
, as an additional plugin output that warns about the performance pitfalls. This has to remain an opt-in, until people stop having zendframework/*
in their dependencies overall.@Ocramius I agree that 2% overhead is ok when it can be removed with a one liner, but as you mentioned this is not documented and i wasn't aware it was so easy to get around it. Thank you for mentioning it.
Question is how to make this more visible now: do you think that adding it to the output of composer install
would suffice?
Seems like the cleanest approach to literally force people to know that this thing is actively doing something.
@Ocramius Just realized for fast requests that are in the 10-20ms range the overhead becomes 10% though :-(
For that, I think a perf optimization is valid.
Perhaps we can add a phpbench/phpbench
test to verify the impact long-term, and patch this stuff:
We rolled this out to production now and monitoring on the autoloader shows the heavy effect, average autoloading time increases from 1 ms to 3ms on a 9ms average response time.
Have you tried rolling out the replace:
suggestion? That is pretty much
the way this is supposed to be handled, until the entire Zend ->
Laminas rename unfortunate fallout (sigh, american copyright laws) is over.
On Mon, May 31, 2021, 17:42 Benjamin Eberlei @.***> wrote:
We rolled this out to production now and monitoring on the autoloader shows the heavy effect:
[image: Screenshot from 2021-05-31 17-39-38] https://user-images.githubusercontent.com/26936/120216584-7eb96080-c237-11eb-9f38-205dde1883f6.png
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/laminas/laminas-zendframework-bridge/issues/83#issuecomment-851567209, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEAXP4YA4DE76EPNXJDTQOU6BANCNFSM452U4GJQ .
I am running into trouble getting it to work in combination with https://github.com/beberlei/composer-monorepo-plugin/ but hope to find a solution soon.
@beberlei I've identified that laminas/laminas-code
no longer needs laminas/laminas-eventmanager
in its core, so I'm releasing a new minor today with the removed dependency (and therefore the performance hit should be gone, if you only need it for the proxying mechanisms)
@beberlei FYI, there's work in progress to completely phase out laminas/laminas-zendframework-bridge
.
For now, we're probing this on some individual packages, like we did in https://github.com/laminas/laminas-stdlib/pull/33
If that proves to be successful, we will go on and implement this for all other packages.
Most Laminas and Mezzio packages have been updated (and released) to remove the dependency on this package.
The only one left, and on which I cannot do anything anymore, are:
Two of the above left: we can then probably phase out this package completely, and call this dark chapter of ZendFramework "done" :|
I think this issue can be closed because the very last PR to stop using this package is about to be merged and released.
Which one is that? 🤔
Awesome, released! 💪
I'd say that this package needs to be abandoned soon-ish then.
I think this issue can be closed because the very last PR to stop using this package is about to be merged and released.
As far as I can see, the last released version of laminas/laminas-server
still includes this. It got removed as dependency about 2 years ago over here, but this didn't get released yet. Maybe that can happen as well soonish? Thanks! 🙂
@hostep send a patch
Closing here: this component is slowly being phased out anyway, so your focus should be to migrate.
Bug Report
Summary
The bridge prepend autoloader runs on all autoloaded classes regardless of what package they are from. The implementation is not very efficient and adds unacceptable overhead of 4ms for a regular Symfony request of 200ms. The problem imho is that a lot of laminas components always include this bridge, even though the README of this component states differently:
Current behavior
Expensive logic detecing class aliasing is run on every class regardless if it starts with
Zend
or not.How to reproduce
Run a Profiler on any script with for example laminas/laminas-event-manager 3.3.1 as a composer dependency, for example any Symfony that includes
friendsofphp/proxy-manager-lts
. Here from Tideways:Expected behavior
At the very least let the autoloader early exit with
if (strpos($class, 'Zend') !== 0) { return; }
.But better would be not to enable this autoloader automatically via
"autoload": {"files":
key.In addition this package should not be included in other laminas packages by default such as
laminas/laminas-eventmanager
: https://github.com/laminas/laminas-eventmanager/blob/3.4.x/composer.json#L27