-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
perf: autoloader #8005
perf: autoloader #8005
Conversation
We can't customize `Config` namespace name.
It is set in Config\Autoload, and it may be renamed.
- When we use Composer, we use Composer autoloader first. So no need to load Composer classmap to CI4 autoloader. - Add `App` and `Config` namespaces to composer.json. So we can use composer dump-autoload for app classes.
1) CodeIgniter\Config\ConfigTest::testCreateNonConfig ErrorException: Constant EVENT_PRIORITY_LOW already defined /home/runner/work/CodeIgniter4/CodeIgniter4/app/Config/Constants.php:84 /home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Factories.php:271 /home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Factories.php:148 /home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Config.php:31 /home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/ConfigTest.php:51
1e75558
to
dbca19c
Compare
Have you done benchmarks on this? Seems like it would be about equivalent. Also feel like it might cause problems with things like namespaces views, but it's been awhile since I've been in that code. I feel like we'd get more performance from simply limiting the namespaces we scan with composer which we can already do. |
One additional thought - more of a historical perspective, I suppose: when I first started the CI4 rewrite one of the goals was to make it not need Composer so that - much like in CI3 - you could run with or without Composer on your system. Though I may have killed that idea once I added in Kint lol. Maybe it's time to get rid of our local autoloader and rely primarily on Composer in v5? Not sure. We're also not working on 5 currently so a moot point I guess. |
The performance is clearly improved in the Welcome page benchmark.
Ubuntu 22.04 + Apache + mod_php
|
It is true if there are a lot of namespaces, but
|
Restricting the number of packages to be scanned raised the numbers a bit, but I are not sure if this is a significant difference. https://codeigniter4.github.io/CodeIgniter4/general/modules.html#specify-composer-packages --- a/app/Config/Modules.php
+++ b/app/Config/Modules.php
@@ -60,7 +60,9 @@ class Modules extends BaseModules
*
* @var array
*/
- public $composerPackages = [];
+ public $composerPackages = [
+ 'only' => [],
+ ];
/**
* --------------------------------------------------------------------------
|
It is not killed because the Zip installation still works. This PR is to give priority to Composer's autoloader when using Composer. Composer's autoloader already has a dump of the class map, and there is no need to copy it to CI's autoloader and use CI's autoloader. |
Thanks for doing those numbers. It had a more significant improvement than I expected. |
The results without Xdebug.
|
4.5 with Composer:
4.5 without Composer (removed
|
0fd30c5
to
d04c5c9
Compare
d04c5c9
to
1ac4ff6
Compare
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change in appstater
?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/admin/starter/composer.json#L27
Added to appstarter composer.json, and updated the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement, and great docs.
Description
To improve the performance of autoloader.
Benchmark: #8005 (comment) and #8005 (comment)
App
andConfig
namespaces tocomposer.json
Before: 1. CI classmap loader, 2. CI PSR-4 autoloader, 3. Composer autoloader
After: 1. Composer autoloader, 2. CI classmap loader, 3. CI PSR-4 autoloader
Checklist: