-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance of ClassAliasLoader #2
Comments
I have just seen that this stuff is used beyond GU::makeInstance. This will be hard |
Wrong!
That is the price you have to pay for backwards compatibility.
That is already the case! See: https://github.com/TYPO3/class-alias-loader/blob/master/src/ClassAliasMapGenerator.php#L113-L117 |
Maybe take a break? 😄 really don't get your point here! |
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 |
This instance is created with
GU = GeneralUtility. |
You have a fully composer based TYPO3 installation? Can you please provide the composer.json for this distribution then?
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. |
By the way: I think this is a task https://typo3.org/extensions/repository/view/compatibility6 should do.
Yes
If you use I totally get what you say, but apparently not vice versa. |
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. 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 |
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. |
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: composer/composer#5174). |
Yes, but my preferred ubiquitous language is the PHP world's language, not TYPO3's 😅
I don't have any use case anymore. (Erstwhile i could have done that because installing TYPO3 as a dependency was buggy AF)
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% |
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.
Well, we need to educate then.
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.
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. |
First improvement would be to re-work the alias map storage to work statically like composer did. |
here is the corresponding ticket on forge: https://forge.typo3.org/issues/75996 |
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
The text was updated successfully, but these errors were encountered: