Use PHP 8.4 lazy ghosts for Dependency injection

[come-nc]

• Resolves: #

Summary

Use the new feature from PHP 8.4 Lazy objects for our Dependency injections system.

It’s not possible to make a ghost for an interface, so it’s only after aliases and so on are resolved that this happens, directly on the class.
It’s active for all classes. It should improve performances either for classes doing stuff in the constructor, or for classes loading a lot of dependencies. Thanks to the lazy ghosts only services which are actually used will be built.
This is only enabled for classes relying on the autobuild, so declarations like:

		$context->registerService(IAuditLogger::class, function (ContainerInterface $c) {
			return new AuditLogger($c->get(ILogFactory::class), $c->get(IConfig::class));
		});

will not benefit from it.
They should be replaced by:

		$context->registerServiceAlias(IAuditLogger::class, AuditLogger::class);

I’ve seen a measurable improvement of performances on some request with this, I would be interested in a more detailed analysis.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

This breaks because we use stuff like this:

		// Make sure that the application class is not loaded before the database is setup
		if ($systemConfig->getValue('installed', false)) {
			$appManager->loadApp('settings');
			/* Build core application to make sure that listeners are registered */
			Server::get(\OC\Core\Application::class);
		}

Here the ghost of \OC\Core\Application will never get instanciated and never run the init sequence in the constructor.
This can be fixed by moving this logic to a method, or by adding a way to flag which classes can be turned into ghost or not. But if we add a switch that’s opt-in we lose a lot of the benefits.

[come-nc]

There were 2 failures:

1) Test\AppFramework\Utility\SimpleContainerTest::testConstructorComplexNoTestParameterFound
Failed asserting that exception of type "OCP\AppFramework\QueryException" is thrown.

2) Test\AppFramework\Utility\SimpleContainerTest::testQueryUntypedNullable
Failed asserting that exception of type "OCP\AppFramework\QueryException" is thrown.

Here the test expects a query to throw because a dependency is not found, but it will only throw if the object is used, not before.
I would say that’s less of a problem, it’s a small behavior change.

[provokateurin]

But if we add a switch that’s opt-in we lose a lot of the benefits

Maybe make it opt out then? I'm not sure how often this pattern occurs as I've never come across it so far.

I think architecture wise it's already pretty bad that we load a class and depend on the code in the constructor to do something without using the object afterwards. Ideally the constructor stuff is moved to a separate method that is then called after Server::get().

[come-nc]

But if we add a switch that’s opt-in we lose a lot of the benefits

Maybe make it opt out then? I'm not sure how often this pattern occurs as I've never come across it so far.

I think architecture wise it's already pretty bad that we load a class and depend on the code in the constructor to do something without using the object afterwards. Ideally the constructor stuff is moved to a separate method that is then called after Server::get().

Yeah, for me opt-out makes no sense either. Adding the opt-out flag or fixing the code to call a method is just as easy.
It seems the core application is the only one relying on this in this repository, cypress tests are passing.
Normal applications implement IBootstrap and rely on the register or the boot method.
The core application is a special case it seems.

But there may be other subtle side effect like this one here and there, so I suppose we’ll need to hide this under an opt-in config flag at the beginning and test it thouroughly before switching. Maybe it can be tested on our instance at some point.

[provokateurin]

Yeah we should make sure to merge it early and test it for a long while and even then we might need to put it behind a config flag and enable it by default in 33 or later...

[come-nc]

Yeah we should make sure to merge it early and test it for a long while and even then we might need to put it behind a config flag and enable it by default in 33 or later...

I thought about this and I’m not sure it’s possible to put this behind a config flag: When the config is read, part of the DI services are already initiated and would not benefit from it.

[come-nc]

In the end I managed to put it behind a config flag. The flag is read early enough that it does not impact the performance boost.

[come-nc]

Note: because the config flag is opt-in, the CI will run the tests without the feature enabled, that’s a bit of a shame 🤔

[provokateurin]

Note: because the config flag is opt-in, the CI will run the tests without the feature enabled, that’s a bit of a shame 🤔

We need to enable it in CI, otherwise we'd miss some things. We also need to do the same for very important and complex apps to check for any issues.

[come-nc]

Note: because the config flag is opt-in, the CI will run the tests without the feature enabled, that’s a bit of a shame 🤔

We need to enable it in CI, otherwise we'd miss some things. We also need to do the same for very important and complex apps to check for any issues.

Should I switch it to opt-out and enabled by default?

[provokateurin]

Should I switch it to opt-out and enabled by default?

Yes and if there are any concerns during RC phase we can still disable it. I just hope we find all problems that might be out there until then.

[come-nc]

Rebased and changed to enabled by default.

[provokateurin]

LGTM, just wondering about the option.

[ChristophWurst]

Here is a comparison of running php occ on master vs on your branch: https://blackfire.io/profiles/compare/a6f5c3df-e077-4342-8c33-b3f4eea51167/graph

I would have expected a significant difference, because the occ commands all have a deep hierarchy of dependencies, and with the ghosts we should instantiate fewer classes.

Did I miss something?

I’ve seen a measurable improvement of performances on some request with this, I would be interested in a more detailed analysis.

Is it something you can share here or privately?

[come-nc]

Here is a comparison of running php occ on master vs on your branch: https://blackfire.io/profiles/compare/a6f5c3df-e077-4342-8c33-b3f4eea51167/graph

I would have expected a significant difference, because the occ commands all have a deep hierarchy of dependencies, and with the ghosts we should instantiate fewer classes.

Did I miss something?

I did not test with occ.
I found blackfire quite unhelpful for this PR, because for some reason it splits Server::query into several lines and it’s not possible to get a clear count of method calls. Or at least I failed to do so.

I think the issue here is that Symfony\Component\Console\Application triggers the loading of each command when add is called, so for all commands, we get the overhead of the ghost with no benefit.
The only benefit we get is from commands dependencies, but I do not know how many of those there are. This is where a clear count of query calls would be useful.

I’ve seen a measurable improvement of performances on some request with this, I would be interested in a more detailed analysis.

Is it something you can share here or privately?

Sending you.

[AndyScherzinger]

🐘