TYPO3 / class-alias-loader

Composer Plugin to provide aliases to PHP classes that have been renamed
MIT License
42 stars 10 forks source link

Improve performance of ClassAliasLoader #2

Open vertexvaar opened 8 years ago

vertexvaar commented 8 years ago

Abstract: The ClassAliasLoader adds unnecessary overhead if autoload_classaliasmap.php is holding no entries and the API is never used.

I set up a TYPO3 7.6.6 installation with the official bootstrap package and profiled the frontend request on the root page. Profiling proves, that 3.5 % of the whole request time is simply "lost" because of the class alias loading, which does not have any impact. This might sound negligible, but most TYPO3 installations i know have 10+ extensions. The more classes, the greater the impact. I also profiled some of these installations and i got results between 2 % and 4 % request time spent on class alias loading. Not talking about percentages: The class alias loading is delaying the request from 2 to 14 ms. BTW: The memory usage impact is insignificant.

I won't hold speeches about the importance of fast websites, user (dis-)satisfaction or else. Just let us fix this.

My proposal: If both arrays are empty, the do not register the ClassAliasLoader, but add a factory callback or such instead of the instance to ClassAliasMap. At the moment the API gets called we can still resolve the ClassAliasLoader and register it as spl_autoload function.

A PR will follow

vertexvaar commented 8 years ago

I have just seen that this stuff is used beyond GU::makeInstance. This will be hard

helhum commented 8 years ago

The ClassAliasLoader adds unnecessary overhead if autoload_classaliasmap.php is holding no entries and the API is never used

Wrong!

Profiling proves, that 3.5 % of the whole request time is simply "lost" because of the class alias loading

That is the price you have to pay for backwards compatibility.

If both arrays are empty, the do not register the ClassAliasLoader

That is already the case! See: https://github.com/TYPO3/class-alias-loader/blob/master/src/ClassAliasMapGenerator.php#L113-L117

helhum commented 8 years ago

I have just seen that this stuff is used beyond GU::makeInstance. This will be hard

Maybe take a break? 😄 really don't get your point here!

helhum commented 8 years ago

Please help me to reproduce your problem.

Do you use a composer based TYPO3 installation? Then please provide the require section of the root composer.json

Do you use a legacy installation? The please provide the result of the command ls typo3conf/ext/*/Migrations/Code/ClassAliasMap.php

vertexvaar commented 8 years ago

That is already the case! See: https://github.com/TYPO3/class-alias-loader/blob/master/src/ClassAliasMapGenerator.php#L113-L117

This instance is created with composer create-project and TYPO3's composer.json has a "always-add-alias-loader": true in it. So these lines will never be used. And i do not doubt, that there are people who use create-project, push the code to prod or copy these parts of the composer.json.

Maybe take a break? 😄 really don't get your point here!

GU = GeneralUtility. GU::makeInstance -> GU:getClassName -> ClassLoadingInformation::getClassNameForAlias -> ClassAliasMap::getClassNameForAlias -> ClassAliasLoader::getClassNameForAlias

helhum commented 8 years ago

This instance is created with composer create-project

You have a fully composer based TYPO3 installation? Can you please provide the composer.json for this distribution then?

and TYPO3's composer.json has a "always-add-alias-loader": true in it

That does not matter in such case, because this config property is only evaluated in the composer.json of the root package (root composer.json): https://github.com/TYPO3/class-alias-loader/blob/master/src/ClassAliasMapGenerator.php#L110

This means, that yes, the distributed tar of TYPO3 will always be slower in terms of class loading, both alias maps wise and in general, because additional class loading info for new packages is added during runtime.

But in a composer distribution, you have none of that. Unless any package defines an alias map, the impact of the typo3/class-alias-loader is zero, as the autoload.php stays untouched.

vertexvaar commented 8 years ago

That is the price you have to pay for backwards compatibility.

By the way: I think this is a task https://typo3.org/extensions/repository/view/compatibility6 should do.

You have a fully composer based TYPO3 installation?

Yes

Can you please provide the composer.json for this distribution then?

https://git.typo3.org/Packages/TYPO3.CMS.git/blob/7a6112192ecdb3f82f59a1d89e075406546b5988:/composer.json

That does not matter in such case, because this config property is only evaluated in the composer.json of the root package (root composer.json)

If you use composer create-project typo3/cms TYPO3's composer.json IS the root composer.json. It is a fully composer enabled TYPO3 distribution. That's the special point of installing TYPO3 as a distribution! If i would define it as dependency, TYPO3's composer.json is not the root composer.json and therefore the class alias loading would be enabled only in case an alias map exists.

I totally get what you say, but apparently not vice versa.

vertexvaar commented 8 years ago

That is the price you have to pay for backwards compatibility.

By the way, i think https://typo3.org/extensions/repository/view/compatibility6 should handle that stuff, not a dependency package, that will always be installed per default. I feel the pain of extension authors updating their code with every mayor release of TYPO3 and i can relate to the decision to build something (this package) that allows hundreds of people to just increase the supported version number instead of forcing them to update their stuff, but on the other hand i think we have reached the end of a long journey from TYPO3 class loading to PSR-0 to PSR-4 and class loading won't change that often/fast anymore. People should just stick to PSR-4 and everything will be fine.

Composer gave us a damn fine class loading and the whole PHP world goes "yeah!" about that. Why overload that, if it's just overhead?

drops the mic

helhum commented 8 years ago

If you use composer create-project typo3/cms?

Why would you want to do so? Maybe describe your exact use case?

TYPO3's composer.json IS the root composer.json

Sure, I know. I only did not come up with the idea, that someone would do composer create-project typo3/cms 😄

It is a fully composer enabled TYPO3 distribution

Nope, it is not. At least not in TYPO3's terms. You get what is packaged as TYPO3 tar distribution, with all disadvantages attached (including the behavior you observe). TYPO3 is in "composer mode" only if typo3/cms is installed as requirement of another package, NEVER if it is installed as root package.

Here is how you can verify a "composer enabled" TYPO3 in the backend dropdown:

composer mode

That's the special point of installing TYPO3 as a distribution!

Please, let me understand what you want to achieve. I'm honestly lost.

If i would define it as dependency, TYPO3's composer.json is not the root composer.json

true

and therefore the class alias loading would be enabled only in case an alias map exists.

true

So why wouldn't you want to define typo3/cms as dependency? composer create-project typo3/cms is really only meant for development only (and packaging of course). Maybe this is the misunderstanding here?

I totally get what you say,

rly?

but apparently not vice versa.

true 😄

helhum commented 8 years ago

drops the mic

Sorry, if I sounded harsh. That is absolutely not my intention. I am really trying to understand what you are saying, what your use case is and how I can help.

I am not saying there is no potential for improvement. I bet there is. But as far as I understood what you described, everything is intended behavior.

helhum commented 8 years ago

By the way, i think https://typo3.org/extensions/repository/view/compatibility6 should handle that stuff, not a dependency package, that will always be installed per default.

This would only be possible, if we would remove the possibility for extension authors to add class alias maps themselves in their extensions. This is certainly something that could be discussed in a place related to further TYPO3 development.

However: the upcoming TYPO3 version 8.x will (afaik) include alias maps itself anyway (to provide BC for Fluid) and maybe in other places as well), so we should rather focus on reducing overhead but still keeping the alias functionality.

I'd appreciate ideas on that! We should, e.g. definitely we should store the alias maps in static arrays (like here: https://github.com/composer/composer/pull/5174).

vertexvaar commented 8 years ago

I totally get what you say,

rly?

Yes, but my preferred ubiquitous language is the PHP world's language, not TYPO3's 😅 I never understood why we (the TYPO3 users, developers and contribs.) use such a different language than the rest of the world.

If you use composer create-project typo3/cms?

Why would you want to do so? Maybe describe your exact use case?

I don't have any use case anymore. (Erstwhile i could have done that because installing TYPO3 as a dependency was buggy AF) But as i said, there are people and i'm 100% sure that these exist, which use create-project or even a tar to build their webpage (maybe they don't know better, not everyone is an expert).

I am not saying there is no potential for improvement. I bet there is.

Indeed, there is a lot of potential, but the most of it was not discovered, yet. I promise you i know what i'm talking about 😉

So far, so good, i'd say let's focus on a solution, but that'd be nonsense if the core itself provides an alias map in the future. Hence i close this one and discontinue the development.

I've got a far better idea, but i'll need to write a proof of concept first. Not promising too much, but i think it does not just remove the 3.5% class alias lookup but should speed up things by around 5% to 8%

helhum commented 8 years ago

I never understood why we (the TYPO3 users, developers and contribs.) use such a different language than the rest of the world.

Legacy, you know. We (unfortunately?) can't force all cool new stuff (like composer) onto people. We need to slowly push them into the direction.

But as i said, there are people and i'm 100% sure that these exist, which use create-project

Well, we need to educate then.

even a tar to build their webpage (maybe they don't know better, not everyone is an expert).

I'm pretty sure about that! That's why we make a difference (other than other composer only projects) and allow users to add extensions at runtime, thus new class loading definitions at runtime. If you have ideas how to improve that situation, which means: improve how class loading info can be added dynamically, you're highly welcome. Since this relates to the integration into TYPO3, I suggest to open a ticket on forge for that.

I've got a far better idea, but i'll need to write a proof of concept first. Not promising too much, but i think it does not just remove the 3.5% class alias lookup but should speed up things by around 5% to 8%

Before you go to the drawing board, please get in touch with me via Slack. I tried to reach you already there. While I absolutely think that there can be performance gains, I also know what optmizations have been tried before, which failed because they did not deliver the full feature set we strive for.

Meanwhile I re-open the ticket here with a renamed description.

helhum commented 8 years ago

First improvement would be to re-work the alias map storage to work statically like composer did. This will give us improved boot times for PHP 5.6 and higher

helhum commented 8 years ago

here is the corresponding ticket on forge: https://forge.typo3.org/issues/75996