Unaccaptable performance hit about laminas-zendframework-bridge

[beberlei]

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.

[Ocramius]

@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 laminas/laminas-stdlib#33

If that proves to be successful, we will go on and implement this for all other packages.

[PowerKiKi]

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:

• mezzio/mezzio-authentication-oauth2#38
• laminas/laminas-xml2json#9
• laminas/laminas-mvc-plugin-fileprg#17
• laminas/laminas-barcode#13

[PowerKiKi]

laminas/laminas-mvc-plugin-fileprg#17 (comment)

[Ocramius]

@hostep send a patch

[Ocramius]

I think this mostly needs documentation improvements:

• the fact that this package is included by default in 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.
• the package can be safely shadowed via "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.
• 2% overhead for something that is removed with a one-liner (where safe to do so) seems OK.

[beberlei]

@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.

[Ocramius]

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.

[beberlei]

@Ocramius Just realized for fast requests that are in the 10-20ms range the overhead becomes 10% though :-(

[Ocramius]

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:

laminas-zendframework-bridge/src/Autoloader.php

Lines 124 to 171 in ff8e9a7

	/**
	* @return callable
	*/
	private static function createAppendAutoloader(array $namespaces, ArrayObject $loaded)
	{
	/**
	* @param string $class Class name to autoload
	* @return void
	*/
	return static function ($class) use ($namespaces, $loaded) {
	$segments = explode('\\', $class);
	if ($segments[0] === 'ZendService' && isset($segments[1])) {
	$segments[0] .= '\\' . $segments[1];
	unset($segments[1]);
	$segments = array_values($segments);
	}
	$i = 0;
	$check = '';
	// We are checking segments of the namespace to match quicker
	while (isset($segments[$i + 1], $namespaces[$check . $segments[$i] . '\\'])) {
	$check .= $segments[$i] . '\\';
	++$i;
	}
	if ($check === '') {
	return;
	}
	$alias = $namespaces[$check]
	. strtr(substr($class, strlen($check)), [
	'Apigility' => 'ApiTools',
	'Expressive' => 'Mezzio',
	'Zend' => 'Laminas',
	'AbstractZendServer' => 'AbstractZendServer',
	'ZendServerDisk' => 'ZendServerDisk',
	'ZendServerShm' => 'ZendServerShm',
	'ZendMonitor' => 'ZendMonitor',
	]);
	$loaded[$alias] = true;
	if (class_exists($alias) || interface_exists($alias) || trait_exists($alias)) {
	class_alias($alias, $class);
	}
	};
	}

[Ocramius]

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 <#83 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEAXP4YA4DE76EPNXJDTQOU6BANCNFSM452U4GJQ> .

[beberlei]

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.

[Ocramius]

@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)

[Ocramius]

Two of the above left: we can then probably phase out this package completely, and call this dark chapter of ZendFramework "done" :|

[PowerKiKi]

I think this issue can be closed because the very last PR to stop using this package is about to be merged and released.

[Ocramius]

Which one is that? 🤔

[Ocramius]

Awesome, released! 💪

I'd say that this package needs to be abandoned soon-ish then.

[hostep]

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! 🙂

[Ocramius]

Closing here: this component is slowly being phased out anyway, so your focus should be to migrate.