Skip to content

Comments

perf(autoloader): Force own autoloader#8578

Merged
nickvergessen merged 2 commits intomasterfrom
perf/autoloader/force-own-autoloader
Jan 25, 2023
Merged

perf(autoloader): Force own autoloader#8578
nickvergessen merged 2 commits intomasterfrom
perf/autoloader/force-own-autoloader

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jan 20, 2023

And don't register any generic PSR-4 nor PSR-0 loader in server.

https://github.com/nextcloud/server/blob/473c546b5c562e19ba8f9d3aef9cd64d2458cda7/lib/private/legacy/OC_App.php#L284-L290

Benchmarks

Blackfire comparison graph on IO wait

Bildschirmfoto vom 2023-01-20 13-51-25

Blackfire recommendations before

Bildschirmfoto vom 2023-01-20 13-53-14

Blackfire recommendations after

Bildschirmfoto vom 2023-01-20 13-50-49

🖼️ Screenshots

🏚️ Before 🏡 After
🦥 🐌

🏁 Checklist

@ChristophWurst ChristophWurst self-assigned this Jan 20, 2023
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jan 20, 2023 •
edited
Loading

metrics.composer.autoload.find_file.count 117 <= 50

Server and this app have authorative autoloaders. But I think the generic autoloader in server tries to load this app's files before the shipped autoloader gets a change. If the generic autoloader isn't registered the shipped autoloader gets a chance to load the file.

@ChristophWurst ChristophWurst mentioned this pull request Jan 20, 2023
Merged
@nickvergessen nickvergessen added this to the 💚 Next Major (26) milestone Jan 20, 2023
@nickvergessen
Copy link
Member

nickvergessen commented Jan 20, 2023

Thanks, I was always tempted to add one, but I disliked the "ah forgot to run bash build/autoload..." from server. I didn't know it's possible to do it for 3rdparty but not the own classes :)

@ChristophWurst ChristophWurst mentioned this pull request Jan 20, 2023
Merged
@nickvergessen
Copy link
Member

nickvergessen commented Jan 23, 2023

Seems CI doesn't like it
grafik

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jan 23, 2023

that means composer i didn't happen

ChristophWurst and others added 2 commits January 25, 2023 09:47
@ChristophWurst @nickvergessen
perf(autoloader): Force own autoloader
0ddc5a5 
And don't register any generic PSR-4 nor PSR-0 loader in server.

Signed-off-by: Christoph Wurst <[email protected]>
@nickvergessen
fix(CI): Run "composer install" in integration tests before enabling …
a2d87ad 
…the app

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen force-pushed the perf/autoloader/force-own-autoloader branch from 398ee88 to a2d87ad Compare January 25, 2023 08:49
@nickvergessen
Copy link
Member

nickvergessen commented Jan 25, 2023

that means composer i didn't happen

Right, it was run only after the app was enabled.
Fixed now

@nickvergessen nickvergessen merged commit fac1de2 into master Jan 25, 2023
@nickvergessen nickvergessen deleted the perf/autoloader/force-own-autoloader branch January 25, 2023 13:40
@ChristophWurst ChristophWurst mentioned this pull request Jan 26, 2023
Merged
6 tasks
@nickvergessen nickvergessen mentioned this pull request Jan 26, 2023
Merged
4 tasks
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 17, 2023

Tip: nextcloud/mail#8020 to relax the autoloader on dev setups and make it find new files too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@vitormattos vitormattos Awaiting requested review from vitormattos

Assignees

@ChristophWurst ChristophWurst

Labels

performance 🚀

Projects

💌 📅 👥 Groupware team
Archived in project

Milestone

v16.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

2 participants

@ChristophWurst @nickvergessen