-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(autoloading): Add missing autoloaders for shipped apps #36112
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
fix(autoloading): Add missing autoloaders for shipped apps #36112
Conversation
PVince81
left a comment
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 catch
any reason why our build autoloader script did not cover those ? otherwise CI would have caught this discrepancy, would be good to add a step there for that
Only apps that have the composer file are checked |
juliusknorr
left a comment
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.
files_external seems to have some issues still according to CI:
Migration step 'OCA\Files_External\Migration\Version1011Date20200630192246' is unknown
15
Added |
Typo in the PSR4 spec |
a54e6f9 to
53a5f35
Compare
8c58025 to
f9b8e2d
Compare
Signed-off-by: Christoph Wurst <[email protected]>
Signed-off-by: Christoph Wurst <[email protected]>
Signed-off-by: Christoph Wurst <[email protected]>
f9b8e2d to
f4ff908
Compare
|
CI is not happy about the theming app changes :s |
|
#36652 is left. This meta PR is obsolete. |
Summary
Shipped apps in this repo have their own authorative (optimized) composer autoloader. But not all of them. The apps still work, but we need a dynamic PSR4 autoloader that has to access the filesystem to check for class existence. Furthermore I'm assuming that with each app autoloader we add, there is a chance that multiple autoloaders try to load a certain file.
By adding the few missing autoloaders I was able to reduce
php occruntime by 1%.GET <host>/status.phpimproved from 330.20 requests per second to 344.20 request per second or 4.2%.The autoloader was missing for the following apps
dashboardperf(autoloading): Add authoritative autoloader for dashboard #36440files_externalperf(autoloading): Add authoritative autoloader for files_external #36469weather_statusperf(autoloading): Add authoritative autoloader for weather-status #36444I assume that distributed Nextcloud setups with the php code on shared storage will see an even bigger performance improvement.
100x
php occbenchmarktime for i in `seq 1 100`; do php occ; doneDetails
100x occ before
real 0m50,214s
user 0m43,513s
sys 0m6,638s
100x occ after
real 0m49,866s
user 0m43,296s
sys 0m6,529s
Apache Bench
ab -n 10000 -c 20 https://localhost/status.phpDetails
Before
After
Checklist