Optimize the check for inlined services

[aschempp]

Our current implementation calls $container->getRemovedIds() for every class before checking if its a singleton. This results in a lot of overhead, because $container->getRemovedIds() loads the removed IDs from file on every access. I assume that's to not memory-cache the IDs, but in our case it results in a ton of unnecessary calls.

The behavior of System::import() and System::importStatic() have slightly changed:
In debug mode, the existing behavior will remain the same. In production, we use getInstance() or try to create a new instance of the class before checking for removed IDs.

[fritzmg]

Is there any debugging benefit from not caching the service IDs in debug mode for the current request?

[leofeyer]

It seems that you are never setting static::$removedServiceIds. 🤔

[aschempp]

Good catch! 😆
I updated to PR to "always cache", because in production one will always immediately get an exception when checking for the services ID, either because it was inlined or because the class does not exist/cannot be instantiated.

[leofeyer]

I find the current logic a little confusing:

if (\is_object($strClass)) {
	// Use object
}
elseif (isset(static::$arrSingletons[$strClass])) {
	// Use object
}
elseif ($container->has($strClass) && (strpos($strClass, '\\') !== false || !class_exists($strClass))) {
	// Get service
}
elseif (($container->getParameter('kernel.debug') || !class_exists($strClass)) && self::isServiceInlined($strClass)) {
	// throw ServiceNotFoundException()
}
elseif (!class_exists($strClass)) {
	// throw RuntimeException('class not found')
}

Why don't we make the "class not found" case more straight-forward?

if (\is_object($strClass)) {
	// Use object
}
elseif (isset(static::$arrSingletons[$strClass])) {
	// Use object
}
elseif ($container->has($strClass) && (strpos($strClass, '\\') !== false || !class_exists($strClass))) {
	// Get service
}
elseif (!class_exists($strClass)) {
	if (self::isServiceInlined($strClass)) {
		// throw ServiceNotFoundException()
	}

	// throw RuntimeException('class not found')
}

[aschempp]

This would not consider the case when a class exists but the service was inlined. If I define a service for a class, I would always assume the service is used. That's why we currently check for that case first, but for performance reasons this PR changes it to only check in dev mode.

[leofeyer]

but for performance reasons this PR changes it to only check in dev mode.

It apparently does more than that, because the prior condition was elseif ($container instanceof Container && isset($container->getRemovedIds()[$strClass])) and had no class_exists() check. I do not really understand what the code does now. 🤷🏻‍♂️