Skip to content
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

Add cache check before registering routes #1367

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

serpentblade
Copy link
Contributor

If routes are already cached, it is not necessary to re-register Horizon routes on every request.

I've found that many Laravel packages don't check for already cached routes and will needlessly re-register routes that already exist in the cache anyway. This reduces boot times in production by ~0.5 ms which adds up when applied to multiple other first-party Laravel packages.

I can open PRs in other projects if this is approved.

If routes are already cached, it is not necessary to re-register Horizon routes on every request.

I've found that many Laravel packages don't check for already cached routes and will needlessly re-register routes that already exist in the cache anyway.  This reduces boot times in production by ~0.5 ms which adds up when applied to multiple other first-party Laravel packages.

I can open PRs in other projects if this is approved.
@driesvints
Copy link
Member

loadRoutesFrom already checks for this. Please check the underlying code.

@driesvints driesvints closed this Jan 15, 2024
@serpentblade
Copy link
Contributor Author

I see that loudRoutesFrom does handle this check already. But that doesn't account for the fact that when routes are cached Routes::group(...) is called only to have nothing happen.

If adding this code is not considered a valid solution, perhaps moving Route::groups(...) into the routes file instead is more acceptable? Ultimately my aim here is to not have this code run at all if it is not required to run.

@driesvints
Copy link
Member

Thanks. I'll leave the decision with @taylorotwell but I don't feel this is necessary.

@driesvints driesvints reopened this Jan 15, 2024
@taylorotwell taylorotwell merged commit 8b76994 into laravel:5.x Jan 15, 2024
23 checks passed
@siarheipashkevich
Copy link

@driesvints @serpentblade will be better to move 'Route::group' inside routes files for avoiding extra logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants