Only load routes of the app which is requested

[MorrisJobke]

This removes ~70-90 ms on every request that involves a URLGenerator (in this case the stats are from the files default view). The total request too ~800ms so this is around 10%.

Before:

After:

Diff:

[nickvergessen]

This will break activity app, because it has to generate links for various other Apps?

[MorrisJobke]

This will break activity app, because it has to generate links for various other Apps?

Not really. It will only load the app for which the URLs where requested. So it does not open all the routes.php files of all apps at once, but if you request files.view.index then only the ones from the files app. Beside that there is often caching involved for the routes + their parameters.

But if you request then settings.user.index it will also load this. But before it always loaded for each generate() request that was not cached all the routes that are available.

[skjnldsv]

Tested and works fine.
Could not find any bug on various apps and core testing

[MorrisJobke]

This might cause problem with OCS routes: https://github.com/nextcloud/server/pull/13714/files#diff-0c5fff2a86ede7f85fc3215283d957fcR113

Let me fix that.

[MorrisJobke]

And I will also add some tests 🙈

[rullzer]

Yeah ocs is different unforntuantly.

Did you also test this with the caching router? As I assume this will then have less effect.

[MorrisJobke]

Did you also test this with the caching router? As I assume this will then have less effect.

It was with caching enabled, but somehow still caused the generation of the URL for each request. Maybe something we should look into separately.

I fixed the OCS stuff and added tests. This is now ready to go in here. Also the OCS routes are just a weird hack and maybe require some more generalization but this is a different topic.

[MorrisJobke]

I know why this failed. The logout URL has the request parameter in the URL:

server/lib/private/legacy/user.php

Lines 272 to 277 in bb86a8c

	$logoutUrl = $urlGenerator->linkToRouteAbsolute(
	'core.login.logout',
	[
	'requesttoken' => \OCP\Util::callRegister(),
	]
	);

And thus the caching does not hit in:

server/lib/private/Route/CachingRouter.php

Line 55 in c1e4f9f

$key = $this->context->getHost() . '#' . $this->context->getBaseUrl() . $name . sha1(json_encode($parameters)) . (int)$absolute;

[icewind1991]

maybe worth adding a fallback to loading all routes?

I don't know if there is any code out there that messes with the route name format

[rullzer]

I know why this failed. The logout URL has the request parameter in the URL:

server/lib/private/legacy/user.php

Lines 272 to 277 in bb86a8c
$logoutUrl = $urlGenerator->linkToRouteAbsolute(
'core.login.logout',
[
'requesttoken' => \OCP\Util::callRegister(),
]
);

And thus the caching does not hit in:

server/lib/private/Route/CachingRouter.php

Line 55 in c1e4f9f
$key = $this->context->getHost() . '#' . $this->context->getBaseUrl() . $name . sha1(json_encode($parameters)) . (int)$absolute;

Right. Yes and it needs to be there becuase it is a default get.
Maybe we should hack the caching router to handle this case better then. As it is special anyways.

[rullzer]

Added the fallback.

Lets get this in!

[MorrisJobke]

Changes by @rullzer make sense 👍

[MorrisJobke]

The change for the routes to be loaded via include instead of include_once is to allow it to properl load them in the unit tests as they re-initiate the routes during the same execution cycle.

[MorrisJobke]

Should be ready for review now :)

[skjnldsv]

🐘

[rullzer]

The change for the routes to be loaded via include instead of include_once is to allow it to properl load them in the unit tests as they re-initiate the routes during the same execution cycle.

So we modify the code to make the test pass? That doesn't sound right. From my understanding include once is correct here.

[MorrisJobke]

The change for the routes to be loaded via include instead of include_once is to allow it to properl load them in the unit tests as they re-initiate the routes during the same execution cycle.

So we modify the code to make the test pass? That doesn't sound right. From my understanding include once is correct here.

Not in the context of changed behavior:

• request files.SOMETHING
• load only the files routes
• return route
• request files_SOMETHING
• can't determine the app and thus need to load all routes
• load all (this includes the files routes again) and because it was built with "it loads anything at once anyways" it is not meant for merging routes it will load all and overwrite the existing stuff in the "load all" step

But I'm fine with refine this and change that. On the other hand: we need to re-profile, because the causing issue seem to be solved as the timing doesn't improve that heavily anymore for me locally.

[MorrisJobke]

So we modify the code to make the test pass? That doesn't sound right. From my understanding include once is correct here.

I reverted the change and instead streamlined the tests a bit (it initialized once the router per test case which then hits the limitations of this implementation due to the include_once - now the router is initialized only once).

[MorrisJobke]

I now properly injected the IRouter into the IURLGenerator. Now the tests are also properly separated.