Skip to content

feat: Cache compiled routes #52793

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 13 commits into from
Jun 5, 2025
Merged

feat: Cache compiled routes #52793

skjnldsv merged 13 commits into from
Jun 5, 2025

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented May 13, 2025 •
edited by sorbaugh
Loading

PR for #54155

Summary

  1. Use symfony tools to compile all routes
  2. Cache that in a local cache
  3. Load from cache instead of parsing routes on each request
  4. Do the same for the generator postponed to follow-up if needed
  5. Check whether cache reset is needed in some situations

Checklist

@come-nc come-nc self-assigned this May 13, 2025
@come-nc come-nc added the 2. developing Work in progress label May 13, 2025
@come-nc come-nc added this to the Nextcloud 32 milestone May 13, 2025
@come-nc
Copy link
Contributor Author

come-nc commented May 13, 2025

Notes:

  1. Big win on core routes like login. ~100ms saved
  2. Smaller win on apps/ routes, ~20ms saved
  3. On apps/ routes, 2MB more memory usage, could be avoided by caching by app, but then we lose part of the time improvement
  4. We lose routes pointing to a closure, because that cannot be serialized. This is not used a lot and mainly used for including files throuh IRoute::actionInclude. We should deprecate that and add a work-around, but I’m short on ideas, we might have to load the routes again for these ones (if we cache in which file they are it might not be that expensive).

@come-nc come-nc requested a review from provokateurin May 14, 2025 09:55
@come-nc come-nc force-pushed the feat/cache-routes branch from abc0a2e to 99b3377 Compare May 14, 2025 20:58
@come-nc
Copy link
Contributor Author

come-nc commented May 16, 2025

  1. Do the same for the generator

I am not sure if it makes sense for generations, as that’s used less often I suppose?
We could still implement in a follow-up if needed.

  1. Check whether cache reset is needed in some situations

Cache has a prefix using applications versions, so we’re fine on this point.

@come-nc come-nc requested a review from ChristophWurst May 16, 2025 15:25
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 16, 2025
@come-nc
Copy link
Contributor Author

come-nc commented May 16, 2025

Ready for reviews. Should not be merged yet so that we can compare with master performances more easily.

provokateurin
provokateurin reviewed May 19, 2025
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed commit-by-commit so it is a bit messy because some commits should have been squashed to make sense 🙈

@come-nc
Copy link
Contributor Author

come-nc commented May 19, 2025

@provokateurin see https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L705-L713

@come-nc come-nc force-pushed the feat/cache-routes branch from ed23cd2 to fb4fe2d Compare May 20, 2025 16:08
ChristophWurst
ChristophWurst approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Didn't test

@come-nc come-nc marked this pull request as ready for review June 2, 2025 14:20
@come-nc come-nc requested a review from a team as a code owner June 2, 2025 14:20
@come-nc come-nc requested review from artonge, skjnldsv and sorbaugh and removed request for a team June 2, 2025 14:20
@come-nc come-nc force-pushed the feat/cache-routes branch from fb4fe2d to c847510 Compare June 2, 2025 14:45
@come-nc
Copy link
Contributor Author

come-nc commented Jun 2, 2025

Failing CI is related.

Current theory:

  • In some cases after enabling an application its routes are missing from cached routes
  • This is supposed to be avoided by the cache prefix based on application versions, but that does not take into account disabled/enabled applications, only the version in DB

I will try to use only enabled applications for the cache prefix.

@come-nc come-nc force-pushed the feat/cache-routes branch from 83d96b3 to 4105837 Compare June 2, 2025 16:37
@come-nc come-nc mentioned this pull request Jun 3, 2025
Merged
4 tasks
@come-nc come-nc force-pushed the feat/cache-routes branch 2 times, most recently from 3ced68c to 69a04e5 Compare June 3, 2025 14:44
@come-nc come-nc force-pushed the feat/cache-routes branch from c72cb36 to 5565eda Compare June 5, 2025 12:32
come-nc added 10 commits June 5, 2025 17:58
@come-nc
feat(router): Cache routes in local cache if possible
ee3abdd 
This is not ideal because serializing the routecollection is not easy.
It seems Symfony has its own way of doing things by dumping routes to a
 PHP file, maybe that would be better, but it would mean pulling a new
 symfony dependency and maybe refactor our Router.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
feat(router): Use Symfony CompiledUrlMatcherDumper to cache routes
01575b7 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Use a more fitting cache key
5c9092b 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Deprecate action and actionInclude from routes and fix actionInc…
216da3a 
…lude

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
feat(router): Add fallback for legacy action and file routes
446b22a 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Fix psalm issue by adding missing parameter for profiler
0d78c60 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Use only enabled applications versions in the cache prefix
e8370bf 
This makes sure the cached routes are updated after enabling a
 previously disabled application

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
chore(tests): Adapt AppManager test to the use of searchValues
a2fdecc 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Fix log display for integration tests
c6f2eff 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
chore(tests): Add test for AppConfig::getAppInstalledVersions
89f51a8 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the feat/cache-routes branch from 5565eda to 89f51a8 Compare June 5, 2025 15:59
come-nc added 2 commits June 5, 2025 18:03
@come-nc
fix: Use application ids as well as versions in the cache prefix
12e7f46 
Avoids a corner case issue if one application was disabled and another
 one enabled with the same version, just to be safe.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
fix: Load all partially enabled apps routes
bb485fb 
This avoids cache issues if some apps are only enabled for some groups,
 by loading the routes from all apps enabled for anyone, not only the
 current user.
If the queried application is disabled for the current user based on
 groups, the middleware will cancel the query.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Jun 5, 2025
@come-nc
chore: update psalm baseline
ba53147 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc mentioned this pull request Jun 5, 2025
Draft
4 tasks
@come-nc come-nc enabled auto-merge June 5, 2025 16:42
@skjnldsv skjnldsv disabled auto-merge June 5, 2025 18:05
@skjnldsv skjnldsv merged commit b8b229c into master Jun 5, 2025
205 of 207 checks passed
@skjnldsv skjnldsv deleted the feat/cache-routes branch June 5, 2025 18:05
susnux
susnux reviewed Jun 11, 2025
View reviewed changes
lib/private/Route/CachingRouter.php
if (!$cachedRoutes) {
parent::loadRoutes();
$cachedRoutes = $this->serializeRouteCollection($this->root);
$this->cache->set($key, $cachedRoutes, 3600);
Copy link
Contributor

@susnux susnux Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key does not include any version number, so this probably means its always cached also on updates?
Because then there is no way to flush on update if the routes have changed, no?

Copy link
Contributor Author

@come-nc come-nc Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the cache have a global prefix with all apps version numbers, see

server/lib/private/Server.php

Line 605 in acc2311

$prefixClosure = function () use ($logQuery, $serverVersion): ?string {

@come-nc come-nc mentioned this pull request Jun 12, 2025
Open
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
@provokateurin provokateurin mentioned this pull request Dec 2, 2025
Closed
8 tasks
@danxuliu danxuliu mentioned this pull request Dec 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@provokateurin provokateurin provokateurin approved these changes

@artonge artonge Awaiting requested review from artonge artonge is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update performance 🚀

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

6 participants

@come-nc @ChristophWurst @susnux @skjnldsv @provokateurin